diff mbox

udp: Fix potential wrong ip_hdr(skb) pointers

Message ID 1233914158.21135.11.camel@localhost.localdomain
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Jesper Dangaard Brouer Feb. 6, 2009, 9:55 a.m. UTC 
On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> Please respin this patch of your's with proper commit message
> and signoffs, thanks!

Like the UDP header fix, pskb_may_pull() can potentially
alter the SKB buffer.  Thus the saddr and daddr, pointers
may point to the old skb->data buffer.

I haven't seen corruptions, as its only seen if the old
skb->data buffer were reallocated by another user and
written into very quickly (or poison'd by SLAB debugging).

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
---

 net/ipv4/udp.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller Feb. 6, 2009, 9:59 a.m. UTC | #1 
From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Fri, 06 Feb 2009 10:55:58 +0100

> On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> > Please respin this patch of your's with proper commit message
> > and signoffs, thanks!
> 
> Like the UDP header fix, pskb_may_pull() can potentially
> alter the SKB buffer.  Thus the saddr and daddr, pointers
> may point to the old skb->data buffer.
> 
> I haven't seen corruptions, as its only seen if the old
> skb->data buffer were reallocated by another user and
> written into very quickly (or poison'd by SLAB debugging).
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

Applied, thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Feb. 6, 2009, 10:04 a.m. UTC | #2 
Jesper Dangaard Brouer a écrit :
> On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
>> Please respin this patch of your's with proper commit message
>> and signoffs, thanks!
> 
> Like the UDP header fix, pskb_may_pull() can potentially
> alter the SKB buffer.  Thus the saddr and daddr, pointers
> may point to the old skb->data buffer.
> 

I dont know... daddr and saddr are not pointers but integers.

Patch makes sense as a cleanup, but ChangeLog seems wrong ?

> I haven't seen corruptions, as its only seen if the old
> skb->data buffer were reallocated by another user and
> written into very quickly (or poison'd by SLAB debugging).
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> ---
> 
>  net/ipv4/udp.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index cc3a0a0..c47c989 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1234,8 +1234,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  	struct udphdr *uh;
>  	unsigned short ulen;
>  	struct rtable *rt = (struct rtable*)skb->dst;
> -	__be32 saddr = ip_hdr(skb)->saddr;
> -	__be32 daddr = ip_hdr(skb)->daddr;
> +	__be32 saddr, daddr;
>  	struct net *net = dev_net(skb->dev);
>  
>  	/*
> @@ -1259,6 +1258,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  	if (udp4_csum_init(skb, uh, proto))
>  		goto csum_error;
>  
> +	saddr = ip_hdr(skb)->saddr;
> +	daddr = ip_hdr(skb)->daddr;
> +
>  	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
>  		return __udp4_lib_mcast_deliver(net, skb, uh,
>  				saddr, daddr, udptable);
> 
>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesper Dangaard Brouer Feb. 6, 2009, 10:49 a.m. UTC | #3 
On Fri, 2009-02-06 at 11:04 +0100, Eric Dumazet wrote:
> Jesper Dangaard Brouer a écrit :
> > On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> >> Please respin this patch of your's with proper commit message
> >> and signoffs, thanks!
> > 
> > Like the UDP header fix, pskb_may_pull() can potentially
> > alter the SKB buffer.  Thus the saddr and daddr, pointers
> > may point to the old skb->data buffer.
> > 
> 
> I dont know... daddr and saddr are not pointers but integers.

Yes, you are right... its only in the ipv6 code these are pointers
(which as DaveM mentioned handels it correctly).

> Patch makes sense as a cleanup, but ChangeLog seems wrong ?

Okay, lets view it as a cleanup... Its upto DaveM if he wants to fix the
commit message (or ask me the correct it, revert and reapply...)


> > I haven't seen corruptions, as its only seen if the old
> > skb->data buffer were reallocated by another user and
> > written into very quickly (or poison'd by SLAB debugging).
> > 
> > Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
> > ---
> > 
> >  net/ipv4/udp.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index cc3a0a0..c47c989 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1234,8 +1234,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >  	struct udphdr *uh;
> >  	unsigned short ulen;
> >  	struct rtable *rt = (struct rtable*)skb->dst;
> > -	__be32 saddr = ip_hdr(skb)->saddr;
> > -	__be32 daddr = ip_hdr(skb)->daddr;
> > +	__be32 saddr, daddr;
> >  	struct net *net = dev_net(skb->dev);
> >  
> >  	/*
> > @@ -1259,6 +1258,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >  	if (udp4_csum_init(skb, uh, proto))
> >  		goto csum_error;
> >  
> > +	saddr = ip_hdr(skb)->saddr;
> > +	daddr = ip_hdr(skb)->daddr;
> > +
> >  	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
> >  		return __udp4_lib_mcast_deliver(net, skb, uh,
> >  				saddr, daddr, udptable);
> > 
> >
> 
>
David Miller Feb. 6, 2009, 11:11 a.m. UTC | #4 
From: Jesper Dangaard Brouer <jdb@comx.dk>
Date: Fri, 06 Feb 2009 11:49:22 +0100

> On Fri, 2009-02-06 at 11:04 +0100, Eric Dumazet wrote:
> > Jesper Dangaard Brouer a écrit :
> > > On Fri, 2009-02-06 at 01:08 -0800, David Miller wrote:
> > >> Please respin this patch of your's with proper commit message
> > >> and signoffs, thanks!
> > > 
> > > Like the UDP header fix, pskb_may_pull() can potentially
> > > alter the SKB buffer.  Thus the saddr and daddr, pointers
> > > may point to the old skb->data buffer.
> > > 
> > 
> > I dont know... daddr and saddr are not pointers but integers.
> 
> Yes, you are right... its only in the ipv6 code these are pointers
> (which as DaveM mentioned handels it correctly).
> 
> > Patch makes sense as a cleanup, but ChangeLog seems wrong ?
> 
> Okay, lets view it as a cleanup... Its upto DaveM if he wants to fix the
> commit message (or ask me the correct it, revert and reapply...)

I already pushed the commit out, rebasing the tree is not an
option and a revert is super ugly so it's staying as-is.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index cc3a0a0..c47c989 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1234,8 +1234,7 @@  int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	struct udphdr *uh;
 	unsigned short ulen;
 	struct rtable *rt = (struct rtable*)skb->dst;
-	__be32 saddr = ip_hdr(skb)->saddr;
-	__be32 daddr = ip_hdr(skb)->daddr;
+	__be32 saddr, daddr;
 	struct net *net = dev_net(skb->dev);
 
 	/*
@@ -1259,6 +1258,9 @@  int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	if (udp4_csum_init(skb, uh, proto))
 		goto csum_error;
 
+	saddr = ip_hdr(skb)->saddr;
+	daddr = ip_hdr(skb)->daddr;
+
 	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
 		return __udp4_lib_mcast_deliver(net, skb, uh,
 				saddr, daddr, udptable);