diff mbox series

[net] bareudp: Fix invalid read beyond skb's linear data

Message ID 7741c46545c6ef02e70c80a9b32814b22d9616b3.1628264975.git.gnault@redhat.com (mailing list archive)
State Accepted
Commit 143a8526ab5fd4f8a0c4fe2a9cb28c181dc5a95f
Delegated to: Netdev Maintainers
Headers show
Series [net] bareudp: Fix invalid read beyond skb's linear data | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 23 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/header_inline success Link

Commit Message

Guillaume Nault Aug. 6, 2021, 3:52 p.m. UTC
Data beyond the UDP header might not be part of the skb's linear data.
Use skb_copy_bits() instead of direct access to skb->data+X, so that
we read the correct bytes even on a fragmented skb.

Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 drivers/net/bareudp.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski Aug. 6, 2021, 11:22 p.m. UTC | #1
On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> Data beyond the UDP header might not be part of the skb's linear data.
> Use skb_copy_bits() instead of direct access to skb->data+X, so that
> we read the correct bytes even on a fragmented skb.
> 
> Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
>  drivers/net/bareudp.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> index a7ee0af1af90..54e321a695ce 100644
> --- a/drivers/net/bareudp.c
> +++ b/drivers/net/bareudp.c
> @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>  		family = AF_INET6;
>  
>  	if (bareudp->ethertype == htons(ETH_P_IP)) {
> -		struct iphdr *iphdr;
> +		__u8 ipversion;
>  
> -		iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> -		if (iphdr->version == 4) {
> -			proto = bareudp->ethertype;
> -		} else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> +		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> +				  sizeof(ipversion))) {

No preference just curious - could skb_header_pointer() be better suited?

> +			bareudp->dev->stats.rx_dropped++;
> +			goto drop;
> +		}
> +		ipversion >>= 4;
> +
> +		if (ipversion == 4) {
> +			proto = htons(ETH_P_IP);
> +		} else if (ipversion == 6 && bareudp->multi_proto_mode) {
>  			proto = htons(ETH_P_IPV6);
>  		} else {
>  			bareudp->dev->stats.rx_dropped++;
Guillaume Nault Aug. 8, 2021, 4:16 p.m. UTC | #2
On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> > Data beyond the UDP header might not be part of the skb's linear data.
> > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > we read the correct bytes even on a fragmented skb.
> > 
> > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> >  drivers/net/bareudp.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > index a7ee0af1af90..54e321a695ce 100644
> > --- a/drivers/net/bareudp.c
> > +++ b/drivers/net/bareudp.c
> > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> >  		family = AF_INET6;
> >  
> >  	if (bareudp->ethertype == htons(ETH_P_IP)) {
> > -		struct iphdr *iphdr;
> > +		__u8 ipversion;
> >  
> > -		iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > -		if (iphdr->version == 4) {
> > -			proto = bareudp->ethertype;
> > -		} else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > +		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > +				  sizeof(ipversion))) {
> 
> No preference just curious - could skb_header_pointer() be better suited?

I have no preference either. I just used skb_copy_bits() because it
didn't seem useful to get a pointer to the buffer (just to read one
byte of data).

But I don't mind reposting with skb_header_pointer() if anyone prefers
that solution.
Jakub Kicinski Aug. 9, 2021, 4:19 p.m. UTC | #3
On Sun, 8 Aug 2021 18:16:25 +0200 Guillaume Nault wrote:
> On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> > On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:  
> > > Data beyond the UDP header might not be part of the skb's linear data.
> > > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > > we read the correct bytes even on a fragmented skb.
> > > 
> > > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > > ---
> > >  drivers/net/bareudp.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > > index a7ee0af1af90..54e321a695ce 100644
> > > --- a/drivers/net/bareudp.c
> > > +++ b/drivers/net/bareudp.c
> > > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > >  		family = AF_INET6;
> > >  
> > >  	if (bareudp->ethertype == htons(ETH_P_IP)) {
> > > -		struct iphdr *iphdr;
> > > +		__u8 ipversion;
> > >  
> > > -		iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > > -		if (iphdr->version == 4) {
> > > -			proto = bareudp->ethertype;
> > > -		} else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > > +		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > > +				  sizeof(ipversion))) {  
> > 
> > No preference just curious - could skb_header_pointer() be better suited?  
> 
> I have no preference either. I just used skb_copy_bits() because it
> didn't seem useful to get a pointer to the buffer (just to read one
> byte of data).

Right, the advantage would be in the "fast" case of skb_header_pointer()
being inlined.

> But I don't mind reposting with skb_header_pointer() if anyone prefers
> that solution.
patchwork-bot+netdevbpf@kernel.org Aug. 9, 2021, 10:40 p.m. UTC | #4
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 6 Aug 2021 17:52:06 +0200 you wrote:
> Data beyond the UDP header might not be part of the skb's linear data.
> Use skb_copy_bits() instead of direct access to skb->data+X, so that
> we read the correct bytes even on a fragmented skb.
> 
> Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> 
> [...]

Here is the summary with links:
  - [net] bareudp: Fix invalid read beyond skb's linear data
    https://git.kernel.org/netdev/net/c/143a8526ab5f

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Guillaume Nault Aug. 10, 2021, 9:33 a.m. UTC | #5
On Mon, Aug 09, 2021 at 09:19:18AM -0700, Jakub Kicinski wrote:
> On Sun, 8 Aug 2021 18:16:25 +0200 Guillaume Nault wrote:
> > On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> > > On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:  
> > > > Data beyond the UDP header might not be part of the skb's linear data.
> > > > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > > > we read the correct bytes even on a fragmented skb.
> > > > 
> > > > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > > > ---
> > > >  drivers/net/bareudp.c | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > > > index a7ee0af1af90..54e321a695ce 100644
> > > > --- a/drivers/net/bareudp.c
> > > > +++ b/drivers/net/bareudp.c
> > > > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > > >  		family = AF_INET6;
> > > >  
> > > >  	if (bareudp->ethertype == htons(ETH_P_IP)) {
> > > > -		struct iphdr *iphdr;
> > > > +		__u8 ipversion;
> > > >  
> > > > -		iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > > > -		if (iphdr->version == 4) {
> > > > -			proto = bareudp->ethertype;
> > > > -		} else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > > > +		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > > > +				  sizeof(ipversion))) {  
> > > 
> > > No preference just curious - could skb_header_pointer() be better suited?  
> > 
> > I have no preference either. I just used skb_copy_bits() because it
> > didn't seem useful to get a pointer to the buffer (just to read one
> > byte of data).
> 
> Right, the advantage would be in the "fast" case of skb_header_pointer()
> being inlined.

Yes indeed. The problem was found because of some automated functionnal
tests, not because of any practical use cases. So I didn't consider the
possible performance differences.

I see that you've applied the patch as is already. I can switch to
skb_header_pointer() in the future, if anyone sees any practical
benefit for it.

Thanks for the review.
diff mbox series

Patch

diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index a7ee0af1af90..54e321a695ce 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -71,12 +71,18 @@  static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		family = AF_INET6;
 
 	if (bareudp->ethertype == htons(ETH_P_IP)) {
-		struct iphdr *iphdr;
+		__u8 ipversion;
 
-		iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
-		if (iphdr->version == 4) {
-			proto = bareudp->ethertype;
-		} else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
+		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
+				  sizeof(ipversion))) {
+			bareudp->dev->stats.rx_dropped++;
+			goto drop;
+		}
+		ipversion >>= 4;
+
+		if (ipversion == 4) {
+			proto = htons(ETH_P_IP);
+		} else if (ipversion == 6 && bareudp->multi_proto_mode) {
 			proto = htons(ETH_P_IPV6);
 		} else {
 			bareudp->dev->stats.rx_dropped++;