* Re: KASAN: stack-out-of-bounds Read in xfrm_selector_match (2)
[not found] <0000000000009fc91605afd40d89@google.com>
@ 2020-09-25 3:07 ` Herbert Xu
2020-09-25 4:42 ` [PATCH] xfrm: Use correct address family in xfrm_state_find Herbert Xu
2020-09-28 2:21 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Paul Moore
0 siblings, 2 replies; 4+ messages in thread
From: Herbert Xu @ 2020-09-25 3:07 UTC (permalink / raw)
To: syzbot
Cc: davem, kuba, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs, James Morris, Serge E. Hallyn,
linux-security-module
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: syzbot+577fbac3145a6eb2e7a5@syzkaller.appspotmail.com
>
> ==================================================================
> 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: syzbot+577fbac3145a6eb2e7a5@syzkaller.appspotmail.com
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] xfrm: Use correct address family in xfrm_state_find
2020-09-25 3:07 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Herbert Xu
@ 2020-09-25 4:42 ` Herbert Xu
2020-09-28 5:07 ` Steffen Klassert
2020-09-28 2:21 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Paul Moore
1 sibling, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2020-09-25 4:42 UTC (permalink / raw)
To: syzbot
Cc: davem, kuba, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs, James Morris, Serge E. Hallyn,
linux-security-module
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: syzbot+577fbac3145a6eb2e7a5@syzkaller.appspotmail.com
Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: KASAN: stack-out-of-bounds Read in xfrm_selector_match (2)
2020-09-25 3:07 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Herbert Xu
2020-09-25 4:42 ` [PATCH] xfrm: Use correct address family in xfrm_state_find Herbert Xu
@ 2020-09-28 2:21 ` Paul Moore
1 sibling, 0 replies; 4+ messages in thread
From: Paul Moore @ 2020-09-28 2:21 UTC (permalink / raw)
To: Herbert Xu
Cc: syzbot, davem, kuba, linux-kernel, netdev, steffen.klassert,
syzkaller-bugs, James Morris, Serge E. Hallyn,
linux-security-module
On Thu, Sep 24, 2020 at 11:08 PM Herbert Xu <herbert@gondor.apana.org.au> 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: syzbot+577fbac3145a6eb2e7a5@syzkaller.appspotmail.com
> >
> > ==================================================================
> > 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
www.paul-moore.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfrm: Use correct address family in xfrm_state_find
2020-09-25 4:42 ` [PATCH] xfrm: Use correct address family in xfrm_state_find Herbert Xu
@ 2020-09-28 5:07 ` Steffen Klassert
0 siblings, 0 replies; 4+ messages in thread
From: Steffen Klassert @ 2020-09-28 5:07 UTC (permalink / raw)
To: Herbert Xu
Cc: syzbot, davem, kuba, linux-kernel, netdev, syzkaller-bugs,
James Morris, Serge E. Hallyn, linux-security-module
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: syzbot+577fbac3145a6eb2e7a5@syzkaller.appspotmail.com
> Fixes: 48b8d78315bf ("[XFRM]: State selection update to use inner...")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied, thanks a lot Herbert!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-28 5:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0000000000009fc91605afd40d89@google.com>
2020-09-25 3:07 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Herbert Xu
2020-09-25 4:42 ` [PATCH] xfrm: Use correct address family in xfrm_state_find Herbert Xu
2020-09-28 5:07 ` Steffen Klassert
2020-09-28 2:21 ` KASAN: stack-out-of-bounds Read in xfrm_selector_match (2) Paul Moore
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).