Hi Dmitry
I could not reproduce the issue. Might need some specific configuration...
loopback device has proper ethernet header (all 0)
Fault happens in :
0f b6 0c 30 movzbl (%rax,%rsi,1),%ecx
RAX=1ffff1002d14d74a which is RDI>>3, and RSI=dffffc0000000000
Could this be a KASAN problem ?
Hi Dmitry
I could not reproduce the issue. Might need some specific configuration...
loopback device has proper ethernet header (all 0)
Fault happens in :
0f b6 0c 30 movzbl (%rax,%rsi,1),%ecx
RAX=1ffff1002d14d74a which is RDI>>3, and RSI=dffffc0000000000
Could this be a KASAN problem ?
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Thanks!
--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
From: Eric Dumazet <hidden> Date: 2016-11-26 20:05:20
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Well, I think the intent is to accept a negative number.
This definitely was assumed by commit e1f165032c8bade authors !
I guess they were using a 32bit kernel for their tests.
From: Eric Dumazet <hidden> Date: 2016-11-26 20:34:49
2016-11-26 12:05 GMT-08:00 Eric Dumazet [off-list ref]:
quoted
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Well, I think the intent is to accept a negative number.
This definitely was assumed by commit e1f165032c8bade authors !
I guess they were using a 32bit kernel for their tests.
Correct fix would be to use
skb_push(skb, -skb_network_offset(skb));
As done in other locations...
Hi Dmitry
I could not reproduce the issue. Might need some specific configuration...
loopback device has proper ethernet header (all 0)
Fault happens in :
0f b6 0c 30 movzbl (%rax,%rsi,1),%ecx
RAX=1ffff1002d14d74a which is RDI>>3, and RSI=dffffc0000000000
Could this be a KASAN problem ?
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Thanks!
I can not reproduce the bug on my hosts.
Quite hard to debug for me.
skb_network_offset() can not be negative at this point, unless there is
a bug upper in the stack.
Hannes, do you have an idea of what could be wrong in IPv6 stack ?
Thanks.
Hi Dmitry
I could not reproduce the issue. Might need some specific configuration...
loopback device has proper ethernet header (all 0)
Fault happens in :
0f b6 0c 30 movzbl (%rax,%rsi,1),%ecx
RAX=1ffff1002d14d74a which is RDI>>3, and RSI=dffffc0000000000
Could this be a KASAN problem ?
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Thanks!
I can not reproduce the bug on my hosts.
Quite hard to debug for me.
skb_network_offset() can not be negative at this point, unless there is
a bug upper in the stack.
Hi Eric,
As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().
Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.
I hope that helps.
Hannes, do you have an idea of what could be wrong in IPv6 stack ?
Thanks.
--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Hi Dmitry
I could not reproduce the issue. Might need some specific configuration...
loopback device has proper ethernet header (all 0)
Fault happens in :
0f b6 0c 30 movzbl (%rax,%rsi,1),%ecx
RAX=1ffff1002d14d74a which is RDI>>3, and RSI=dffffc0000000000
Could this be a KASAN problem ?
Hi Eric,
The crash happens when the kernel tries to access shadow for nonmapped memory.
The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.
I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Thanks!
I can not reproduce the bug on my hosts.
Quite hard to debug for me.
skb_network_offset() can not be negative at this point, unless there is
a bug upper in the stack.
Hi Eric,
As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().
Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.
Eric,
Is it enough to debug? Or maybe Andrey can trace some values for you.
From: Eric Dumazet <hidden> Date: 2016-11-28 19:48:51
On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
quoted
Hi Eric,
As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().
Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.
Eric,
Is it enough to debug? Or maybe Andrey can trace some values for you.
Well, now we are talking, if you tell me how many modules you load, it
might help ;)
nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
explain why I could not reproduce the bug.
Let me try ;)
From: Eric Dumazet <hidden> Date: 2016-11-28 21:07:08
On Mon, 2016-11-28 at 11:47 -0800, Eric Dumazet wrote:
On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
quoted
On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
quoted
quoted
Hi Eric,
As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().
Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.
Eric,
Is it enough to debug? Or maybe Andrey can trace some values for you.
Well, now we are talking, if you tell me how many modules you load, it
might help ;)
nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
explain why I could not reproduce the bug.
Let me try ;)
Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")
Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?
(Like about 48 byte pulled :(, and/or skb->csum changed )
From: Eric Dumazet <hidden> Date: 2016-11-28 21:19:18
On Mon, 2016-11-28 at 13:05 -0800, Eric Dumazet wrote:
quoted hunk
On Mon, 2016-11-28 at 11:47 -0800, Eric Dumazet wrote:
quoted
On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
quoted
On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
quoted
quoted
Hi Eric,
As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().
Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.
Eric,
Is it enough to debug? Or maybe Andrey can trace some values for you.
Well, now we are talking, if you tell me how many modules you load, it
might help ;)
nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
explain why I could not reproduce the bug.
Let me try ;)
Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")
Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?
(Like about 48 byte pulled :(, and/or skb->csum changed )
Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")
Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?
ipv4 definitely frees malformed packets.
In general, I think netfilter should avoid 'silent' drops if possible
and let skb continue, but of course such skbs should not be made worse
as what we ate to begin with...
quoted
(Like about 48 byte pulled :(, and/or skb->csum changed )
I think this warrants a review of ipv6 reassembly too, bug reported here
is because ipv6 nf defrag is also done on output.
From: Eric Dumazet <hidden> Date: 2016-11-28 22:15:15
On Mon, 2016-11-28 at 22:34 +0100, Florian Westphal wrote:
Eric Dumazet [off-list ref] wrote:
quoted
quoted
Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")
Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?
ipv4 definitely frees malformed packets.
In general, I think netfilter should avoid 'silent' drops if possible
and let skb continue, but of course such skbs should not be made worse
as what we ate to begin with...
quoted
quoted
(Like about 48 byte pulled :(, and/or skb->csum changed )
I think this warrants a review of ipv6 reassembly too, bug reported here
is because ipv6 nf defrag is also done on output.
On Mon, 2016-11-28 at 22:34 +0100, Florian Westphal wrote:
quoted
Eric Dumazet [off-list ref] wrote:
quoted
quoted
Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")
Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?
ipv4 definitely frees malformed packets.
In general, I think netfilter should avoid 'silent' drops if possible
and let skb continue, but of course such skbs should not be made worse
as what we ate to begin with...
quoted
quoted
(Like about 48 byte pulled :(, and/or skb->csum changed )
I think this warrants a review of ipv6 reassembly too, bug reported here
is because ipv6 nf defrag is also done on output.
Yes, sorry. nf_ct_frag6_queue is mostly derived from ip6_frag_queue
so any bugs in one might also exist in other.
Thats all I wanted to say here. I'll check this tomorrow.
quoted
Looks good, we'll need to change some of the errno return codes in
nf_ct_frag6_gather to 0 though for this to work, which should not be too
hard ;)
If the goal is to let buggy packets pass, then we might need to undo
changes in nf_ct_frag6_queue()
It currently returns -EINVAL in cases where skb wasn't changed/altered
(e.g. because it doesn't have a fragment header), so we should ACCEPT in
that case.
As for 'buggy' packet, I think its ok to mimic ip6_frag_queue, i.e.
if it tosses returning NF_DROP under same circumstance seems ok.
(Passing however will -- on ingress side -- cause snmp stat increments
in ipv6 reassembly, this still might be desireable).
I'll check where undo might be possible/not too hard.
Thanks Eric for debugging this!
From: Eric Dumazet <hidden> Date: 2016-11-28 23:17:24
On Mon, 2016-11-28 at 23:19 +0100, Florian Westphal wrote:
It currently returns -EINVAL in cases where skb wasn't changed/altered
(e.g. because it doesn't have a fragment header), so we should ACCEPT in
that case.
Maybe nf_ct_frag6_queue() should return direct NF_ codes then ...
On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet [off-list ref] wrote:
quoted
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Well, I think the intent is to accept a negative number.
I'm not sure that was the intent since it results in a signedness
issue which leads to an out-of-bounds.
A quick grep shows that the same issue can potentially happen in
multiple places across the kernel:
net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
A similar thing also happened to somebody else (on a receive path!):
https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
Does it make sense to check skb_network_offset() before passing it to
skb_pull() everywhere?
This definitely was assumed by commit e1f165032c8bade authors !
I guess they were using a 32bit kernel for their tests.
--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
From: Eric Dumazet <hidden> Date: 2016-11-29 14:58:37
On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet [off-list ref] wrote:
quoted
quoted
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Well, I think the intent is to accept a negative number.
I'm not sure that was the intent since it results in a signedness
issue which leads to an out-of-bounds.
Hey, I already mentioned where was the bug.
You missed the investigation where I pointed it to FLorian ?
A quick grep shows that the same issue can potentially happen in
multiple places across the kernel:
net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
A similar thing also happened to somebody else (on a receive path!):
https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
Does it make sense to check skb_network_offset() before passing it to
skb_pull() everywhere?
Well, sure, we could add safety checks everywhere and slow the kernel
when debugging is requested.
But skb_network_offset() is not the problem here. Why are you focusing
on it ?
The real problem is in __skb_pull() or __skb_push() and all similar
helpers. Lots of added checks and slowdowns.
On Tue, Nov 29, 2016 at 3:58 PM, Eric Dumazet [off-list ref] wrote:
On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
quoted
On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet [off-list ref] wrote:
quoted
quoted
I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.
Well, I think the intent is to accept a negative number.
I'm not sure that was the intent since it results in a signedness
issue which leads to an out-of-bounds.
Hey, I already mentioned where was the bug.
You missed the investigation where I pointed it to FLorian ?
quoted
A quick grep shows that the same issue can potentially happen in
multiple places across the kernel:
net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
A similar thing also happened to somebody else (on a receive path!):
https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
Does it make sense to check skb_network_offset() before passing it to
skb_pull() everywhere?
Well, sure, we could add safety checks everywhere and slow the kernel
when debugging is requested.
But skb_network_offset() is not the problem here. Why are you focusing
on it ?
The real problem is in __skb_pull() or __skb_push() and all similar
helpers. Lots of added checks and slowdowns.
The issue is not with skb_network_offset(), but with __skb_pull()
using skb_network_offset() as an argument.
I'm not sure what would be the beast way to fix this, to add a check
before every __skb_pull(skb_network_offset()), to fix __skb_pull() to
work with signed ints, to add BUG_ON()'s in __skb_pull, or something
else.
What I meant is that you fixed this very instance of the bug, and I'm
pointing out that a similar one might hit us again.
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
From: Eric Dumazet <hidden> Date: 2016-11-29 16:18:33
On Tue, 2016-11-29 at 16:31 +0100, Andrey Konovalov wrote:
=
The issue is not with skb_network_offset(), but with __skb_pull()
using skb_network_offset() as an argument.
No. The issue can happen with _any_ __skb_pull() with a 'negative'
argument, on 64bit arches.
skb_network_offset() is only one of the many cases this could happen if
a bug is added at some random place, including memory corruption from
a different kernel layer, or buggy hardware.
I'm not sure what would be the beast way to fix this, to add a check
before every __skb_pull(skb_network_offset()), to fix __skb_pull() to
work with signed ints, to add BUG_ON()'s in __skb_pull, or something
else.
What I meant is that you fixed this very instance of the bug, and I'm
pointing out that a similar one might hit us again.
As I said, adding a check in skb_network_offset() would not be generic
enough.
Sure, we can be proactive and add tests everywhere in the kernel, but we
also want to keep it reasonably fast.