Hello,
syzbot found the following issue on:
HEAD commit: eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad5900000
kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180
dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5
compiler: gcc (GCC) 10.1.0-syz 20200507
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
==================================================================
BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877 [inline]
BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match net/xfrm/xfrm_policy.c:216 [inline]
BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60 net/xfrm/xfrm_policy.c:229
Read of size 2 at addr ffffc9001914f55c by task syz-executor.4/15633
CPU: 0 PID: 15633 Comm: syz-executor.4 Not tainted 5.9.0-rc5-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x198/0x1fd lib/dump_stack.c:118
print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
xfrm_flowi_dport include/net/xfrm.h:877 [inline]
__xfrm6_selector_match net/xfrm/xfrm_policy.c:216 [inline]
xfrm_selector_match+0xf36/0xf60 net/xfrm/xfrm_policy.c:229
xfrm_state_look_at.constprop.0+0x144/0x3f0 net/xfrm/xfrm_state.c:1022
xfrm_state_find+0x1a16/0x4d50 net/xfrm/xfrm_state.c:1092
xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:2384 [inline]
xfrm_tmpl_resolve+0x2f3/0xd40 net/xfrm/xfrm_policy.c:2429
xfrm_resolve_and_create_bundle+0x123/0x2590 net/xfrm/xfrm_policy.c:2719
xfrm_lookup_with_ifid+0x235/0x2130 net/xfrm/xfrm_policy.c:3053
xfrm_lookup net/xfrm/xfrm_policy.c:3177 [inline]
xfrm_lookup_route+0x36/0x1e0 net/xfrm/xfrm_policy.c:3188
ip_route_output_flow+0xa6/0xc0 net/ipv4/route.c:2769
udp_sendmsg+0x1a16/0x26c0 net/ipv4/udp.c:1201
udpv6_sendmsg+0x14dd/0x2b90 net/ipv6/udp.c:1344
inet6_sendmsg+0x99/0xe0 net/ipv6/af_inet6.c:638
sock_sendmsg_nosec net/socket.c:651 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:671
____sys_sendmsg+0x331/0x810 net/socket.c:2353
___sys_sendmsg+0xf3/0x170 net/socket.c:2407
__sys_sendmmsg+0x195/0x480 net/socket.c:2497
__do_sys_sendmmsg net/socket.c:2526 [inline]
__se_sys_sendmmsg net/socket.c:2523 [inline]
__x64_sys_sendmmsg+0x99/0x100 net/socket.c:2523
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45d5f9
Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8aa009cc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
RAX: ffffffffffffffda RBX: 0000000000027a40 RCX: 000000000045d5f9
RDX: 00000000000005c3 RSI: 0000000020000240 RDI: 0000000000000004
RBP: 000000000118cf88 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118cf4c
R13: 000000000169fb6f R14: 00007f8aa009d9c0 R15: 000000000118cf4c
addr ffffc9001914f55c is located in stack of task syz-executor.4/15633 at offset 220 in frame:
udp_sendmsg+0x0/0x26c0 net/ipv4/udp.c:2708
this frame has 5 objects:
[32, 40) 'rt'
[64, 104) 'ipc'
[144, 200) 'fl4_stack'
[240, 296) 'cork'
[336, 408) 'opt_copy'
Memory state around the buggy address:
ffffc9001914f400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffc9001914f480: f1 f1 f1 f1 00 f2 f2 f2 00 00 00 00 00 f2 f2 f2
>ffffc9001914f500: f2 f2 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 00 00
^
ffffc9001914f580: 00 00 00 00 00 f2 f2 f2 f2 f2 00 00 00 00 00 00
ffffc9001914f600: 00 00 00 f3 f3 f3 f3 f3 00 00 00 00 00 00 00 00
==================================================================
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
On Mon, Sep 21, 2020 at 07:56:20AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad5900000
> kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180
> dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5
> compiler: gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: [email protected]
>
> ==================================================================
> BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877 [inline]
> BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match net/xfrm/xfrm_policy.c:216 [inline]
> BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60 net/xfrm/xfrm_policy.c:229
> Read of size 2 at addr ffffc9001914f55c by task syz-executor.4/15633
This is yet another ipv4 mapped ipv6 address with IPsec socket policy
combination bug, and I'm sure it is not the last one. We could fix this
one by adding another check to match the address family of the policy
and the SA selector, but maybe it is better to think about how this
should work at all.
We can have only one socket policy for each direction and that
policy accepts either ipv4 or ipv6. We treat this ipv4 mapped ipv6
address as ipv4 and pass it down the ipv4 stack, so this dual usage
will not work with a socket policy. Maybe we can require IPV6_V6ONLY
for sockets with policy attached. Thoughts?
On Thu, Sep 24, 2020 at 09:40:26AM +0200, Steffen Klassert wrote:
>
> This is yet another ipv4 mapped ipv6 address with IPsec socket policy
> combination bug, and I'm sure it is not the last one. We could fix this
> one by adding another check to match the address family of the policy
> and the SA selector, but maybe it is better to think about how this
> should work at all.
>
> We can have only one socket policy for each direction and that
> policy accepts either ipv4 or ipv6. We treat this ipv4 mapped ipv6
> address as ipv4 and pass it down the ipv4 stack, so this dual usage
> will not work with a socket policy. Maybe we can require IPV6_V6ONLY
> for sockets with policy attached. Thoughts?
I'm looking at the history of this and it used to work at the start
because you'd always interpret the flow object with a family. This
appears to have been lost with 8444cf712c5f71845cba9dc30d8f530ff0d5ff83.
I'm working on a fix.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Sep 24, 2020 at 05:43:51PM +1000, Herbert Xu wrote:
> On Thu, Sep 24, 2020 at 09:40:26AM +0200, Steffen Klassert wrote:
> >
> > This is yet another ipv4 mapped ipv6 address with IPsec socket policy
> > combination bug, and I'm sure it is not the last one. We could fix this
> > one by adding another check to match the address family of the policy
> > and the SA selector, but maybe it is better to think about how this
> > should work at all.
> >
> > We can have only one socket policy for each direction and that
> > policy accepts either ipv4 or ipv6. We treat this ipv4 mapped ipv6
> > address as ipv4 and pass it down the ipv4 stack, so this dual usage
> > will not work with a socket policy. Maybe we can require IPV6_V6ONLY
> > for sockets with policy attached. Thoughts?
>
> I'm looking at the history of this and it used to work at the start
> because you'd always interpret the flow object with a family. This
> appears to have been lost with 8444cf712c5f71845cba9dc30d8f530ff0d5ff83.
I'm sure it can be fixed to work with either ipv4 or ipv6.
If I understand that right, it should be possible to talk
ipv4 and ipv6 through that socket, but the policy will
accept only one address family.
> I'm working on a fix.
Thanks!
On Mon, Sep 21, 2020 at 07:56:20AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad5900000
> kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180
> dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5
> compiler: gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: [email protected]
>
> ==================================================================
> BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877 [inline]
> BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match net/xfrm/xfrm_policy.c:216 [inline]
> BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60 net/xfrm/xfrm_policy.c:229
> Read of size 2 at addr ffffc9001914f55c by task syz-executor.4/15633
>
> CPU: 0 PID: 15633 Comm: syz-executor.4 Not tainted 5.9.0-rc5-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x198/0x1fd lib/dump_stack.c:118
> print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
> __kasan_report mm/kasan/report.c:513 [inline]
> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
> xfrm_flowi_dport include/net/xfrm.h:877 [inline]
This one goes back more than ten years. This patch should fix
it.
---8<---
The struct flowi must never be interpreted by itself as its size
depends on the address family. Therefore it must always be grouped
with its original family value.
In this particular instance, the original family value is lost in
the function xfrm_state_find. Therefore we get a bogus read when
it's coupled with the wrong family which would occur with inter-
family xfrm states.
This patch fixes it by keeping the original family value.
Note that the same bug could potentially occur in LSM through
the xfrm_state_pol_flow_match hook. I checked the current code
there and it seems to be safe for now as only secid is used which
is part of struct flowi_common. But that API should be changed
so that so that we don't get new bugs in the future. We could
do that by replacing fl with just secid or adding a family field.
Reported-by: [email protected]
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 69520ad3d83b..9b5f2c2b9770 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1019,7 +1019,8 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
*/
if (x->km.state == XFRM_STATE_VALID) {
if ((x->sel.family &&
- !xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+ (x->sel.family != family ||
+ !xfrm_selector_match(&x->sel, fl, family))) ||
!security_xfrm_state_pol_flow_match(x, pol, fl))
return;
@@ -1032,7 +1033,9 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
*acq_in_progress = 1;
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
- if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+ if ((!x->sel.family ||
+ (x->sel.family == family &&
+ xfrm_selector_match(&x->sel, fl, family))) &&
security_xfrm_state_pol_flow_match(x, pol, fl))
*error = -ESRCH;
}
@@ -1072,7 +1075,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
if (best || acquire_in_progress)
@@ -1089,7 +1092,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Resend with proper subject.
---8<---
The struct flowi must never be interpreted by itself as its size
depends on the address family. Therefore it must always be grouped
with its original family value.
In this particular instance, the original family value is lost in
the function xfrm_state_find. Therefore we get a bogus read when
it's coupled with the wrong family which would occur with inter-
family xfrm states.
This patch fixes it by keeping the original family value.
Note that the same bug could potentially occur in LSM through
the xfrm_state_pol_flow_match hook. I checked the current code
there and it seems to be safe for now as only secid is used which
is part of struct flowi_common. But that API should be changed
so that so that we don't get new bugs in the future. We could
do that by replacing fl with just secid or adding a family field.
Reported-by: [email protected]
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 69520ad3d83b..9b5f2c2b9770 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1019,7 +1019,8 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
*/
if (x->km.state == XFRM_STATE_VALID) {
if ((x->sel.family &&
- !xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
+ (x->sel.family != family ||
+ !xfrm_selector_match(&x->sel, fl, family))) ||
!security_xfrm_state_pol_flow_match(x, pol, fl))
return;
@@ -1032,7 +1033,9 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
*acq_in_progress = 1;
} else if (x->km.state == XFRM_STATE_ERROR ||
x->km.state == XFRM_STATE_EXPIRED) {
- if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
+ if ((!x->sel.family ||
+ (x->sel.family == family &&
+ xfrm_selector_match(&x->sel, fl, family))) &&
security_xfrm_state_pol_flow_match(x, pol, fl))
*error = -ESRCH;
}
@@ -1072,7 +1075,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
if (best || acquire_in_progress)
@@ -1089,7 +1092,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
tmpl->mode == x->props.mode &&
tmpl->id.proto == x->id.proto &&
(tmpl->id.spi == x->id.spi || !tmpl->id.spi))
- xfrm_state_look_at(pol, x, fl, encap_family,
+ xfrm_state_look_at(pol, x, fl, family,
&best, &acquire_in_progress, &error);
}
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Sep 24, 2020 at 11:08 PM Herbert Xu <[email protected]> wrote:
> On Mon, Sep 21, 2020 at 07:56:20AM -0700, syzbot wrote:
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit: eb5f95f1 Merge tag 's390-5.9-6' of git://git.kernel.org/pu..
> > git tree: upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=13996ad5900000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=ffe85b197a57c180
> > dashboard link: https://syzkaller.appspot.com/bug?extid=577fbac3145a6eb2e7a5
> > compiler: gcc (GCC) 10.1.0-syz 20200507
> >
> > Unfortunately, I don't have any reproducer for this issue yet.
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: [email protected]
> >
> > ==================================================================
> > BUG: KASAN: stack-out-of-bounds in xfrm_flowi_dport include/net/xfrm.h:877 [inline]
> > BUG: KASAN: stack-out-of-bounds in __xfrm6_selector_match net/xfrm/xfrm_policy.c:216 [inline]
> > BUG: KASAN: stack-out-of-bounds in xfrm_selector_match+0xf36/0xf60 net/xfrm/xfrm_policy.c:229
> > Read of size 2 at addr ffffc9001914f55c by task syz-executor.4/15633
> >
> > CPU: 0 PID: 15633 Comm: syz-executor.4 Not tainted 5.9.0-rc5-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x198/0x1fd lib/dump_stack.c:118
> > print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
> > __kasan_report mm/kasan/report.c:513 [inline]
> > kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
> > xfrm_flowi_dport include/net/xfrm.h:877 [inline]
>
> This one goes back more than ten years. This patch should fix
> it.
>
> ---8<---
> The struct flowi must never be interpreted by itself as its size
> depends on the address family. Therefore it must always be grouped
> with its original family value.
>
> In this particular instance, the original family value is lost in
> the function xfrm_state_find. Therefore we get a bogus read when
> it's coupled with the wrong family which would occur with inter-
> family xfrm states.
>
> This patch fixes it by keeping the original family value.
>
> Note that the same bug could potentially occur in LSM through
> the xfrm_state_pol_flow_match hook. I checked the current code
> there and it seems to be safe for now as only secid is used which
> is part of struct flowi_common. But that API should be changed
> so that so that we don't get new bugs in the future. We could
> do that by replacing fl with just secid or adding a family field.
I'm thinking it might be better to pass the family along with the flow
instead of passing just the secid (less worry of passing an incorrect
secid that way). Let me see if I can cobble together a quick patch
for testing before bed ...
--
paul moore
http://www.paul-moore.com
On Fri, Sep 25, 2020 at 02:42:56PM +1000, Herbert Xu wrote:
> Resend with proper subject.
>
> ---8<---
> The struct flowi must never be interpreted by itself as its size
> depends on the address family. Therefore it must always be grouped
> with its original family value.
>
> In this particular instance, the original family value is lost in
> the function xfrm_state_find. Therefore we get a bogus read when
> it's coupled with the wrong family which would occur with inter-
> family xfrm states.
>
> This patch fixes it by keeping the original family value.
>
> Note that the same bug could potentially occur in LSM through
> the xfrm_state_pol_flow_match hook. I checked the current code
> there and it seems to be safe for now as only secid is used which
> is part of struct flowi_common. But that API should be changed
> so that so that we don't get new bugs in the future. We could
> do that by replacing fl with just secid or adding a family field.
>
> Reported-by: [email protected]
> Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
> Signed-off-by: Herbert Xu <[email protected]>
Applied, thanks a lot Herbert!