[bpf-next] bpf, net: Introduce skb_pointer_if_linear().

Commit Message

Alexei Starovoitov July 18, 2023, 11:40 p.m. UTC

From: Alexei Starovoitov <[e-mail]>

Network drivers always call skb_header_pointer() with non-null buffer.
Remove !buffer check to prevent accidental misuse of skb_header_pointer().
Introduce skb_pointer_if_linear() instead.

Reported-by: Jakub Kicinski <[e-mail]>
Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 include/linux/skbuff.h | 10 +++++++++-
 kernel/bpf/helpers.c   |  5 ++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

David Laight July 19, 2023, 1:10 p.m. UTC | #1

From: Alexei Starovoitov
> Sent: 19 July 2023 00:40
> 
> Network drivers always call skb_header_pointer() with non-null buffer.
> Remove !buffer check to prevent accidental misuse of skb_header_pointer().
> Introduce skb_pointer_if_linear() instead.
> 
...
> +static inline void * __must_check
> +skb_pointer_if_linear(const struct sk_buff *skb, int offset, int len)
> +{
> +	if (likely(skb_headlen(skb) - offset >= len))
> +		return skb->data + offset;
> +	return NULL;
> +}

Shouldn't both 'offset' and 'len' be 'unsigned int' ?

The check should probably be written:
		offset + len <= skb_headlen(skb)
so that it fails if 'offset' is also large.
(Provided 'offset + len' itself doesn't wrap.)

I've swapped the order because I prefer conditional to be
	if (variable op constant)
and in this case skb_headlen() is the more constant value.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Alexei Starovoitov July 19, 2023, 4:30 p.m. UTC | #2

On Wed, Jul 19, 2023 at 6:10 AM David Laight <[e-mail]> wrote:
>
> From: Alexei Starovoitov
> > Sent: 19 July 2023 00:40
> >
> > Network drivers always call skb_header_pointer() with non-null buffer.
> > Remove !buffer check to prevent accidental misuse of skb_header_pointer().
> > Introduce skb_pointer_if_linear() instead.
> >
> ...
> > +static inline void * __must_check
> > +skb_pointer_if_linear(const struct sk_buff *skb, int offset, int len)
> > +{
> > +     if (likely(skb_headlen(skb) - offset >= len))
> > +             return skb->data + offset;
> > +     return NULL;
> > +}
>
> Shouldn't both 'offset' and 'len' be 'unsigned int' ?
>
> The check should probably be written:
>                 offset + len <= skb_headlen(skb)
> so that it fails if 'offset' is also large.
> (Provided 'offset + len' itself doesn't wrap.)

I agree that this style is easier to read, but
consistency with skb_header_pointer() trumps all such considerations.

Jakub Kicinski July 19, 2023, 5:20 p.m. UTC | #3

On Tue, 18 Jul 2023 16:40:21 -0700 Alexei Starovoitov wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> Network drivers always call skb_header_pointer() with non-null buffer.
> Remove !buffer check to prevent accidental misuse of skb_header_pointer().
> Introduce skb_pointer_if_linear() instead.
> 
> Reported-by: Jakub Kicinski <[e-mail]>
> Signed-off-by: Alexei Starovoitov <[e-mail]>

Acked-by: Jakub Kicinski <[e-mail]>

Thanks!

[e-mail] July 19, 2023, 5:40 p.m. UTC | #4

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <[e-mail]>:

On Tue, 18 Jul 2023 16:40:21 -0700 you wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> Network drivers always call skb_header_pointer() with non-null buffer.
> Remove !buffer check to prevent accidental misuse of skb_header_pointer().
> Introduce skb_pointer_if_linear() instead.
> 
> Reported-by: Jakub Kicinski <[e-mail]>
> Signed-off-by: Alexei Starovoitov <[e-mail]>
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf, net: Introduce skb_pointer_if_linear().
    https://git.kernel.org/bpf/bpf-next/c/6f5a630d7c57

You are awesome, thank you!

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 91ed66952580..f276d0e9816f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4023,7 +4023,7 @@  __skb_header_pointer(const struct sk_buff *skb, int offset, int len,
 	if (likely(hlen - offset >= len))
 		return (void *)data + offset;
 
-	if (!skb || !buffer || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0))
+	if (!skb || unlikely(skb_copy_bits(skb, offset, buffer, len) < 0))
 		return NULL;
 
 	return buffer;
@@ -4036,6 +4036,14 @@  skb_header_pointer(const struct sk_buff *skb, int offset, int len, void *buffer)
 				    skb_headlen(skb), buffer);
 }
 
+static inline void * __must_check
+skb_pointer_if_linear(const struct sk_buff *skb, int offset, int len)
+{
+	if (likely(skb_headlen(skb) - offset >= len))
+		return skb->data + offset;
+	return NULL;
+}
+
 /**
  *	skb_needs_linearize - check if we need to linearize a given skb
  *			      depending on the given device features.
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 9e80efa59a5d..b8ab3bea71b7 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2239,7 +2239,10 @@  __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
 	case BPF_DYNPTR_TYPE_RINGBUF:
 		return ptr->data + ptr->offset + offset;
 	case BPF_DYNPTR_TYPE_SKB:
-		return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt);
+		if (buffer__opt)
+			return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt);
+		else
+			return skb_pointer_if_linear(ptr->data, ptr->offset + offset, len);
 	case BPF_DYNPTR_TYPE_XDP:
 	{
 		void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);