KASAN: stack-out-of-bounds Read in xfrm_state_find (2)

[syzbot]

Hello,

syzkaller hit the following crash on
36ef71cae353f88fd6e095e2aaa3e5953af1685d
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


BUG: KASAN: stack-out-of-bounds in xfrm_state_find+0x303d/0x3170
net/xfrm/xfrm_state.c:1051
Read of size 4 at addr ffff88003adb7760 by task syzkaller429801/2969

CPU: 3 PID: 2969 Comm: syzkaller429801 Not tainted
4.14.0-rc5-next-20171018+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16 [inline]
dump_stack+0x194/0x257 lib/dump_stack.c:52
print_address_description+0x73/0x250 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351 [inline]
kasan_report+0x25b/0x340 mm/kasan/report.c:409
__asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:429
xfrm_state_find+0x303d/0x3170 net/xfrm/xfrm_state.c:1051
xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:1388 [inline]
xfrm_tmpl_resolve+0x309/0xc00 net/xfrm/xfrm_policy.c:1432
xfrm_resolve_and_create_bundle+0x186/0x24a0 net/xfrm/xfrm_policy.c:1830
xfrm_lookup+0xf0a/0x2540 net/xfrm/xfrm_policy.c:2141
xfrm_lookup_route+0x39/0x1a0 net/xfrm/xfrm_policy.c:2259
ip_route_output_flow+0x7c/0xa0 net/ipv4/route.c:2555
udp_sendmsg+0x19b8/0x2cd0 net/ipv4/udp.c:1022
udpv6_sendmsg+0x743/0x3380 net/ipv6/udp.c:1186
inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:763
sock_sendmsg_nosec net/socket.c:632 [inline]
sock_sendmsg+0xca/0x110 net/socket.c:642
SYSC_sendto+0x352/0x5a0 net/socket.c:1749
SyS_sendto+0x40/0x50 net/socket.c:1717
entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x4350a9
RSP: 002b:00007ffe01d21408 EFLAGS: 00000217 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 00000000004350a9
RDX: 0000000000000000 RSI: 0000000020efcf90 RDI: 0000000000000003
RBP: 0000000000000082 R08: 0000000020efc000 R09: 0000000000000010
R10: 0000000000004090 R11: 0000000000000217 R12: 0000000000000000
R13: 0000000000401a20 R14: 0000000000401ab0 R15: 0000000000000000

The buggy address belongs to the page:
page:ffffea0000eb6dc0 count:0 mapcount:0 mapping: (null) index:0x0
flags: 0x100000000000000()
raw: 0100000000000000 0000000000000000 0000000000000000 00000000ffffffff
raw: 0000000000000000 0000000100000001 0000000000000000 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff88003adb7600: 00 f1 f1 f1 f1 04 f2 f2 f2 f2 f2 f2 f2 00 f2 f2
ffff88003adb7680: f2 f2 f2 f2 f2 f8 f2 f2 f2 f2 f2 f2 f2 00 00 00
> ffff88003adb7700: 00 f2 f2 f2 f2 00 00 00 00 00 00 00 f2 f2 f2 f2
^
ffff88003adb7780: f2 00 00 00 00 00 00 00 00 00 f2 f2 f2 f3 f3 f3
ffff88003adb7800: f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.
Please credit me with: Reported-by: syzbot <syzk...@googlegroups.com>

syzbot will keep track of this bug report.
Once a fix for this bug is committed, please reply to this email with:
#syz fix: exact-commit-title
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line.

[Florian Westphal]

syzbot <bot+19b21aa652248382e2...@syzkaller.appspotmail.com> wrote:

[ cc Thomas Egerer ]

> syzkaller hit the following crash on
> 36ef71cae353f88fd6e095e2aaa3e5953af1685d
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
>
> BUG: KASAN: stack-out-of-bounds in xfrm_state_find+0x303d/0x3170
> net/xfrm/xfrm_state.c:1051
> Read of size 4 at addr ffff88003adb7760 by task syzkaller429801/2969

Seems this was added in
commit 8444cf712c5f71845cba9dc30d8f530ff0d5ff83
("xfrm: Allow different selector family in temporary state").

No idea how to fix this:

struct xfrm_state *
xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
const struct flowi *fl, struct xfrm_tmpl *tmpl,
struct xfrm_policy *pol, int *err,
unsigned short family) // AF_INET
{
[..]
unsigned short encap_family = tmpl->encap_family; // AF_INET6
[..]
h = xfrm_dst_hash(net, daddr, saddr, tmpl->reqid, encap_family);

saddr, daddr point to ipv4 addresses inside an on-stack flowi4 struct,
i.e. they get hashed as ipv6 addresses which then results in invalid stack access.

What is this supposed to do if family != encap_family?

I also don't understand how address comparision is supposed to work in this case,
it seems that if saddr/daddr are v4 and template v6 we compare full ipv6 addresses
(how would that succeed...?) and, if saddr/daddr is v6 add template is v4 we just
compare the first 32bit of the ipv6 addresses...?

This fix silences the reproducer, but I am not sure about it, it looks like it
papers over the real problem...

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1359,16 +1359,19 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
struct xfrm_state **xfrm, unsigned short family)
{
struct net *net = xp_net(policy);
+ xfrm_address_t tmp, daddr, saddr;
int nx;
int i, error;
- xfrm_address_t *daddr = xfrm_flowi_daddr(fl, family);
- xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family);
- xfrm_address_t tmp;
+
+ memset(&saddr, 0, sizeof(saddr));
+ memset(&daddr, 0, sizeof(daddr));
+
+ xfrm_flowi_addr_get(fl, &saddr, &daddr, family);

for (nx = 0, i = 0; i < policy->xfrm_nr; i++) {
struct xfrm_state *x;
- xfrm_address_t *remote = daddr;
- xfrm_address_t *local = saddr;
+ xfrm_address_t *remote = &daddr;
+ xfrm_address_t *local = &saddr;
struct xfrm_tmpl *tmpl = &policy->xfrm_vec[i];

if (tmpl->mode == XFRM_MODE_TUNNEL ||
@@ -1389,8 +1392,8 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,

if (x && x->km.state == XFRM_STATE_VALID) {
xfrm[nx++] = x;
- daddr = remote;
- saddr = local;
+ daddr = *remote;
+ saddr = *local;
continue;
}
if (x) {

[Steffen Klassert]

This looks like yet another IPv4 mapped IPv6 address + socket policy bug.

>
> What is this supposed to do if family != encap_family?

This is the case for interfamily tunnels. Here family is the
address family of the inner packet and encap_family is the
address family for the outer one.

>
> I also don't understand how address comparision is supposed to work in this case,
> it seems that if saddr/daddr are v4 and template v6 we compare full ipv6 addresses
> (how would that succeed...?) and, if saddr/daddr is v6 add template is v4 we just
> compare the first 32bit of the ipv6 addresses...?

When we do tunnel or beet mode, we pass saddr and daddr from the
template to xfrm_state_find(), this should be ok. On transport
mode, we pass the addresses from the flowi, assuming that the
IP addresses (and address family) don't change during transformation.
This assumption is wrong in the IPv4 mapped IPv6 case, packet
is IPv4 and template is IPv6.

I'd propose to use the addresses from the template unconditionally,
like the (untested) patch below does.

Unfortunalely the reproducer does not work with my config,
sendto returns EAGAIN. Could anybody try this patch?


---
net/xfrm/xfrm_policy.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4838329..450bdff 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1360,36 +1360,29 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
struct net *net = xp_net(policy);

int nx;
int i, error;
- xfrm_address_t *daddr = xfrm_flowi_daddr(fl, family);
- xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family);

xfrm_address_t tmp;

for (nx = 0, i = 0; i < policy->xfrm_nr; i++) {
struct xfrm_state *x;
- xfrm_address_t *remote = daddr;
- xfrm_address_t *local = saddr;

+ xfrm_address_t *local;
+ xfrm_address_t *remote;

struct xfrm_tmpl *tmpl = &policy->xfrm_vec[i];

- if (tmpl->mode == XFRM_MODE_TUNNEL ||
- tmpl->mode == XFRM_MODE_BEET) {
- remote = &tmpl->id.daddr;
- local = &tmpl->saddr;
- if (xfrm_addr_any(local, tmpl->encap_family)) {
- error = xfrm_get_saddr(net, fl->flowi_oif,
- &tmp, remote,
- tmpl->encap_family, 0);
- if (error)
- goto fail;
- local = &tmp;
- }
+ remote = &tmpl->id.daddr;
+ local = &tmpl->saddr;
+ if (xfrm_addr_any(local, tmpl->encap_family)) {
+ error = xfrm_get_saddr(net, fl->flowi_oif,
+ &tmp, remote,
+ tmpl->encap_family, 0);
+ if (error)
+ goto fail;
+ local = &tmp;
}

x = xfrm_state_find(remote, local, fl, tmpl, policy, &error, family);

if (x && x->km.state == XFRM_STATE_VALID) {
xfrm[nx++] = x;
- daddr = remote;
- saddr = local;

continue;
}
if (x) {
--
2.7.4

[Florian Westphal]

Steffen Klassert <steffen....@secunet.com> wrote:
> On Wed, Nov 01, 2017 at 11:06:08PM +0100, Florian Westphal wrote:
> > I also don't understand how address comparision is supposed to work in this case,
> > it seems that if saddr/daddr are v4 and template v6 we compare full ipv6 addresses
> > (how would that succeed...?) and, if saddr/daddr is v6 add template is v4 we just
> > compare the first 32bit of the ipv6 addresses...?
>
> When we do tunnel or beet mode, we pass saddr and daddr from the
> template to xfrm_state_find(), this should be ok. On transport
> mode, we pass the addresses from the flowi, assuming that the
> IP addresses (and address family) don't change during transformation.
> This assumption is wrong in the IPv4 mapped IPv6 case, packet
> is IPv4 and template is IPv6.

Right, sendto() uses ipv4 address on ipv6 socket.

> I'd propose to use the addresses from the template unconditionally,
> like the (untested) patch below does.
>
> Unfortunalely the reproducer does not work with my config,
> sendto returns EAGAIN. Could anybody try this patch?

The reproducer no longer causes KASAN spew with your patch,
but i don't have a test case that actually creates/uses a tunnel.

[Steffen Klassert]

On Thu, Nov 02, 2017 at 01:25:28PM +0100, Florian Westphal wrote:
> Steffen Klassert <steffen....@secunet.com> wrote:
>
> > I'd propose to use the addresses from the template unconditionally,
> > like the (untested) patch below does.
> >
> > Unfortunalely the reproducer does not work with my config,
> > sendto returns EAGAIN. Could anybody try this patch?
>
> The reproducer no longer causes KASAN spew with your patch,
> but i don't have a test case that actually creates/uses a tunnel.

The patch passed my standard tests, so I tend apply it
after a day in the ipsec/testing branch.

[Steffen Klassert]

FYI: I've just applied the patch below to the ipsec tree.

Subject: [PATCH ipsec] xfrm: Fix stack-out-of-bounds read in xfrm_state_find.

When we do tunnel or beet mode, we pass saddr and daddr from the

template to xfrm_state_find(), this is ok. On transport mode,

we pass the addresses from the flowi, assuming that the IP
addresses (and address family) don't change during transformation.
This assumption is wrong in the IPv4 mapped IPv6 case, packet

is IPv4 and template is IPv6. Fix this by using the addresses
from the template unconditionally.

Signed-off-by: Steffen Klassert <steffen....@secunet.com>

---
net/xfrm/xfrm_policy.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c

index a2e531b..6eb228a 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1361,36 +1361,29 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,

[Dmitry Vyukov]

On Mon, Nov 6, 2017 at 11:16 AM, Steffen Klassert
<steffen....@secunet.com> wrote:
> On Fri, Nov 03, 2017 at 01:10:12PM +0100, Steffen Klassert wrote:
>> On Thu, Nov 02, 2017 at 01:25:28PM +0100, Florian Westphal wrote:
>> > Steffen Klassert <steffen....@secunet.com> wrote:
>> >
>> > > I'd propose to use the addresses from the template unconditionally,
>> > > like the (untested) patch below does.
>> > >
>> > > Unfortunalely the reproducer does not work with my config,
>> > > sendto returns EAGAIN. Could anybody try this patch?
>> >
>> > The reproducer no longer causes KASAN spew with your patch,
>> > but i don't have a test case that actually creates/uses a tunnel.
>>
>> The patch passed my standard tests, so I tend apply it
>> after a day in the ipsec/testing branch.
>
> FYI: I've just applied the patch below to the ipsec tree.

Thanks

Let's tell the bot what fixes this:

#syz fix: xfrm: Fix stack-out-of-bounds read in xfrm_state_find.

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20171106101646.GG23855%40secunet.com.
> For more options, visit https://groups.google.com/d/optout.

[Steffen Klassert]

On Mon, Nov 06, 2017 at 11:16:46AM +0100, Steffen Klassert wrote:
>
> Subject: [PATCH ipsec] xfrm: Fix stack-out-of-bounds read in xfrm_state_find.
>
> When we do tunnel or beet mode, we pass saddr and daddr from the
> template to xfrm_state_find(), this is ok. On transport mode,
> we pass the addresses from the flowi, assuming that the IP
> addresses (and address family) don't change during transformation.
> This assumption is wrong in the IPv4 mapped IPv6 case, packet
> is IPv4 and template is IPv6. Fix this by using the addresses
> from the template unconditionally.
>
> Signed-off-by: Steffen Klassert <steffen....@secunet.com>

I had to revert this, it broke transport mode when the policy
template has no src and dst addresses configured. I'll come up
with some other fix, probably don't do policy/flow maching
when a socket policies address family does not match the
flow address family. This should hopefully fix this whole
class of IPv4 mapped IPv6 with socket policy bugs.