Alexey Kashavkin <akashavkin@xxxxxxxxx> wrote:
> There is an incorrect calculation in the offset variable which causes the nft_skb_copy_to_reg() function to always return -EFAULT. Adding the start variable is redundant. In the __ip_options_compile() function the correct offset is specified when finding the function. There is no need to add the size of the iphdr structure to the offset.
Fixes: dbb5281a1f84 ("netfilter: nf_tables: add support for matching IPv4 options")
But there are more bugs remaining, see below.
> @@ -85,7 +85,6 @@ static int ipv4_find_option(struct net *net, struct sk_buff *skb,
> unsigned char optbuf[sizeof(struct ip_options) + 40];
> struct ip_options *opt = (struct ip_options *)optbuf;
This is wrong, the array should be aligned to fit struct
requirements, so u32 is needed, or __aligned annotation is needed
for optbuf.
> - if (skb_copy_bits(skb, start, opt->__data, optlen))
> + if (skb_copy_bits(skb, sizeof(struct iphdr), opt->__data, optlen))
> return -EBADMSG;
This copies to local on stack buffer.
> if (found)
> - *offset = opt->srr + start;
> + *offset = opt->srr;
This returns a pointer to local on stack buffer.
But that buffer isn't valid anymore when the function
returns.
It might work in case compiler inlines, but better
not rely on this.
Can you make a second patch that places optbuf in the
stack frame of the calling function instead?
