* [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
@ 2013-09-03 17:29 Daniel Borkmann
2013-09-03 17:48 ` Eric Dumazet
2013-09-04 18:57 ` David Miller
0 siblings, 2 replies; 10+ messages in thread
From: Daniel Borkmann @ 2013-09-03 17:29 UTC (permalink / raw)
To: davem; +Cc: netdev, Eric Dumazet
In tcp_v6_do_rcv() code, when processing pkt options, we soley work
on our skb clone opt_skb that we've created earlier before entering
tcp_rcv_established() on our way. However, only in condition ...
if (np->rxopt.bits.rxtclass)
np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
... we work on skb itself. As we extract every other information out
of opt_skb in ipv6_pktoptions path, this seems wrong, since skb can
already be released by tcp_rcv_established() earlier on. When we try
to access it in ipv6_hdr(), we will dereference freed skb.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/tcp_ipv6.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6e1649d..eeb4cb0 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1427,7 +1427,7 @@ ipv6_pktoptions:
if (np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim)
np->mcast_hops = ipv6_hdr(opt_skb)->hop_limit;
if (np->rxopt.bits.rxtclass)
- np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
+ np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(opt_skb));
if (ipv6_opt_accepted(sk, opt_skb)) {
skb_set_owner_r(opt_skb, sk);
opt_skb = xchg(&np->pktoptions, opt_skb);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 17:29 [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv Daniel Borkmann
@ 2013-09-03 17:48 ` Eric Dumazet
2013-09-03 19:46 ` Jean Sacren
2013-09-04 18:57 ` David Miller
1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2013-09-03 17:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Jiri Benc
On Tue, 2013-09-03 at 19:29 +0200, Daniel Borkmann wrote:
> In tcp_v6_do_rcv() code, when processing pkt options, we soley work
> on our skb clone opt_skb that we've created earlier before entering
> tcp_rcv_established() on our way. However, only in condition ...
>
> if (np->rxopt.bits.rxtclass)
> np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
>
> ... we work on skb itself. As we extract every other information out
> of opt_skb in ipv6_pktoptions path, this seems wrong, since skb can
> already be released by tcp_rcv_established() earlier on. When we try
> to access it in ipv6_hdr(), we will dereference freed skb.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/ipv6/tcp_ipv6.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6e1649d..eeb4cb0 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1427,7 +1427,7 @@ ipv6_pktoptions:
> if (np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim)
> np->mcast_hops = ipv6_hdr(opt_skb)->hop_limit;
> if (np->rxopt.bits.rxtclass)
> - np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
> + np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(opt_skb));
> if (ipv6_opt_accepted(sk, opt_skb)) {
> skb_set_owner_r(opt_skb, sk);
> opt_skb = xchg(&np->pktoptions, opt_skb);
Bug added in commit 4c507d2897bd9b
("net: implement IP_RECVTOS for IP_PKTOPTIONS")
CC Jiri
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 17:48 ` Eric Dumazet
@ 2013-09-03 19:46 ` Jean Sacren
2013-09-03 20:35 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Jean Sacren @ 2013-09-03 19:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Daniel Borkmann, davem, netdev, yoshfuji
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 03 Sep 2013 10:48:33 -0700
>
> On Tue, 2013-09-03 at 19:29 +0200, Daniel Borkmann wrote:
> > In tcp_v6_do_rcv() code, when processing pkt options, we soley work
> > on our skb clone opt_skb that we've created earlier before entering
> > tcp_rcv_established() on our way. However, only in condition ...
> >
> > if (np->rxopt.bits.rxtclass)
> > np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
> >
> > ... we work on skb itself. As we extract every other information out
> > of opt_skb in ipv6_pktoptions path, this seems wrong, since skb can
> > already be released by tcp_rcv_established() earlier on. When we try
> > to access it in ipv6_hdr(), we will dereference freed skb.
> >
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> > net/ipv6/tcp_ipv6.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index 6e1649d..eeb4cb0 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -1427,7 +1427,7 @@ ipv6_pktoptions:
> > if (np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim)
> > np->mcast_hops = ipv6_hdr(opt_skb)->hop_limit;
> > if (np->rxopt.bits.rxtclass)
> > - np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
> > + np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(opt_skb));
> > if (ipv6_opt_accepted(sk, opt_skb)) {
> > skb_set_owner_r(opt_skb, sk);
> > opt_skb = xchg(&np->pktoptions, opt_skb);
>
> Bug added in commit 4c507d2897bd9b
> ("net: implement IP_RECVTOS for IP_PKTOPTIONS")
>
> CC Jiri
>
> Acked-by: Eric Dumazet <edumazet@google.com>
You made a mistake.
It was introduced in commit e7219858a ("ipv6: Use ipv6_get_dsfield()
instead of ipv6_tclass()").
Cc the right party.
--
Jean Sacren
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 19:46 ` Jean Sacren
@ 2013-09-03 20:35 ` Eric Dumazet
2013-09-03 21:59 ` Jean Sacren
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2013-09-03 20:35 UTC (permalink / raw)
To: Jean Sacren; +Cc: Daniel Borkmann, davem, netdev, yoshfuji, Jiri Benc
On Tue, 2013-09-03 at 13:46 -0600, Jean Sacren wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 03 Sep 2013 10:48:33 -0700
> >
> > On Tue, 2013-09-03 at 19:29 +0200, Daniel Borkmann wrote:
> > > In tcp_v6_do_rcv() code, when processing pkt options, we soley work
> > > on our skb clone opt_skb that we've created earlier before entering
> > > tcp_rcv_established() on our way. However, only in condition ...
> > >
> > > if (np->rxopt.bits.rxtclass)
> > > np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
> > >
> > > ... we work on skb itself. As we extract every other information out
> > > of opt_skb in ipv6_pktoptions path, this seems wrong, since skb can
> > > already be released by tcp_rcv_established() earlier on. When we try
> > > to access it in ipv6_hdr(), we will dereference freed skb.
> > >
> > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > > ---
> > > net/ipv6/tcp_ipv6.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > > index 6e1649d..eeb4cb0 100644
> > > --- a/net/ipv6/tcp_ipv6.c
> > > +++ b/net/ipv6/tcp_ipv6.c
> > > @@ -1427,7 +1427,7 @@ ipv6_pktoptions:
> > > if (np->rxopt.bits.rxhlim || np->rxopt.bits.rxohlim)
> > > np->mcast_hops = ipv6_hdr(opt_skb)->hop_limit;
> > > if (np->rxopt.bits.rxtclass)
> > > - np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
> > > + np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(opt_skb));
> > > if (ipv6_opt_accepted(sk, opt_skb)) {
> > > skb_set_owner_r(opt_skb, sk);
> > > opt_skb = xchg(&np->pktoptions, opt_skb);
> >
> > Bug added in commit 4c507d2897bd9b
> > ("net: implement IP_RECVTOS for IP_PKTOPTIONS")
> >
> > CC Jiri
> >
> > Acked-by: Eric Dumazet <edumazet@google.com>
>
> You made a mistake.
Really ?
>
> It was introduced in commit e7219858a ("ipv6: Use ipv6_get_dsfield()
> instead of ipv6_tclass()").
>
> Cc the right party.
>
Nope, I did no mistake.
The bug was really added in commit 4c507d2897bd9b as I said.
Then commit e7219858a just did no change : skb was used before and after
the patch
- np->rcv_tclass = ipv6_tclass(ipv6_hdr(skb));
+ np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
How did you get your conclusion ?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 20:35 ` Eric Dumazet
@ 2013-09-03 21:59 ` Jean Sacren
2013-09-03 22:25 ` Daniel Borkmann
2013-09-03 22:29 ` Eric Dumazet
0 siblings, 2 replies; 10+ messages in thread
From: Jean Sacren @ 2013-09-03 21:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Daniel Borkmann, davem, netdev, yoshfuji, Jiri Benc
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 03 Sep 2013 13:35:17 -0700
>
> How did you get your conclusion ?
If one changes one line of code, doesn't one own that line?
--
Jean Sacren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 21:59 ` Jean Sacren
@ 2013-09-03 22:25 ` Daniel Borkmann
2013-09-03 22:29 ` Eric Dumazet
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2013-09-03 22:25 UTC (permalink / raw)
To: Jean Sacren; +Cc: Eric Dumazet, davem, netdev, yoshfuji, Jiri Benc
On 09/03/2013 11:59 PM, Jean Sacren wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 03 Sep 2013 13:35:17 -0700
>>
>> How did you get your conclusion ?
>
> If one changes one line of code, doesn't one own that line?
The question is not about who currently "owns" the line, but who
first *introduced* an error.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 21:59 ` Jean Sacren
2013-09-03 22:25 ` Daniel Borkmann
@ 2013-09-03 22:29 ` Eric Dumazet
2013-09-04 0:51 ` Jean Sacren
1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2013-09-03 22:29 UTC (permalink / raw)
To: Jean Sacren; +Cc: Daniel Borkmann, davem, netdev, yoshfuji, Jiri Benc
On Tue, 2013-09-03 at 15:59 -0600, Jean Sacren wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 03 Sep 2013 13:35:17 -0700
> >
> > How did you get your conclusion ?
>
> If one changes one line of code, doesn't one own that line?
>
'Owning' like he is the guy who can sell it ? OK I understand now
so many people send 'cleanups' ;)
Someone adding a space is not responsible for the real bug.
You'll have to be very careful to do proper attributions.
This is _critical_ for proper stable submissions.
In this case we want the fix to reach linux-3.4 and further versions.
The commit you pointed out was for linux-3.9, and added no bug.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 22:29 ` Eric Dumazet
@ 2013-09-04 0:51 ` Jean Sacren
2013-09-04 6:26 ` Jiri Benc
0 siblings, 1 reply; 10+ messages in thread
From: Jean Sacren @ 2013-09-04 0:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Daniel Borkmann, davem, netdev, yoshfuji, Jiri Benc
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 03 Sep 2013 15:29:12 -0700
>
> On Tue, 2013-09-03 at 15:59 -0600, Jean Sacren wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 03 Sep 2013 13:35:17 -0700
> > >
> > > How did you get your conclusion ?
> >
> > If one changes one line of code, doesn't one own that line?
> >
>
> 'Owning' like he is the guy who can sell it ? OK I understand now
> so many people send 'cleanups' ;)
'Owning' like he is the guy who is responsible for the change. Whether
you understand it or not, that's your own business.
The idea was by taking one example to determine who is really to blame
for: Is the one passing on the bug responsible? Thanks to Daniel
Borkmann for the answer.
--
Jean Sacren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-04 0:51 ` Jean Sacren
@ 2013-09-04 6:26 ` Jiri Benc
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Benc @ 2013-09-04 6:26 UTC (permalink / raw)
To: Jean Sacren; +Cc: Eric Dumazet, Daniel Borkmann, davem, netdev, yoshfuji
On Tue, 3 Sep 2013 18:51:57 -0600, Jean Sacren wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 03 Sep 2013 15:29:12 -0700
> >
> > On Tue, 2013-09-03 at 15:59 -0600, Jean Sacren wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Tue, 03 Sep 2013 13:35:17 -0700
> > > >
> > > > How did you get your conclusion ?
> > >
> > > If one changes one line of code, doesn't one own that line?
> > >
> >
> > 'Owning' like he is the guy who can sell it ? OK I understand now
> > so many people send 'cleanups' ;)
>
> 'Owning' like he is the guy who is responsible for the change. Whether
> you understand it or not, that's your own business.
>
> The idea was by taking one example to determine who is really to blame
> for: Is the one passing on the bug responsible? Thanks to Daniel
> Borkmann for the answer.
Eric is correct, the bug was introduced by me.
Sorry for it. Daniel, thanks for fixing it.
Acked-by: Jiri Benc <jbenc@redhat.com>
--
Jiri Benc
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv
2013-09-03 17:29 [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv Daniel Borkmann
2013-09-03 17:48 ` Eric Dumazet
@ 2013-09-04 18:57 ` David Miller
1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2013-09-04 18:57 UTC (permalink / raw)
To: dborkman; +Cc: netdev, eric.dumazet
From: Daniel Borkmann <dborkman@redhat.com>
Date: Tue, 3 Sep 2013 19:29:12 +0200
> In tcp_v6_do_rcv() code, when processing pkt options, we soley work
> on our skb clone opt_skb that we've created earlier before entering
> tcp_rcv_established() on our way. However, only in condition ...
>
> if (np->rxopt.bits.rxtclass)
> np->rcv_tclass = ipv6_get_dsfield(ipv6_hdr(skb));
>
> ... we work on skb itself. As we extract every other information out
> of opt_skb in ipv6_pktoptions path, this seems wrong, since skb can
> already be released by tcp_rcv_established() earlier on. When we try
> to access it in ipv6_hdr(), we will dereference freed skb.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-09-04 18:57 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 17:29 [PATCH net] net: ipv6: tcp: fix potential use after free in tcp_v6_do_rcv Daniel Borkmann
2013-09-03 17:48 ` Eric Dumazet
2013-09-03 19:46 ` Jean Sacren
2013-09-03 20:35 ` Eric Dumazet
2013-09-03 21:59 ` Jean Sacren
2013-09-03 22:25 ` Daniel Borkmann
2013-09-03 22:29 ` Eric Dumazet
2013-09-04 0:51 ` Jean Sacren
2013-09-04 6:26 ` Jiri Benc
2013-09-04 18:57 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).