Hi,
I've got the following error report while fuzzing the kernel with syzkaller.
On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
A reproducer and .config are attached.
When subtracting rq->sadb_x_ipsecrequest_len from len it can become
negative and the while loop condition remains true.
==================================================================
BUG: KASAN: slab-out-of-bounds in parse_ipsecrequest
net/key/af_key.c:1897 [inline] at addr ffff88006b1e3d0b
BUG: KASAN: slab-out-of-bounds in parse_ipsecrequests+0xc89/0xdc0
net/key/af_key.c:1949 at addr ffff88006b1e3d0b
Read of size 1 by task syz-executor5/11796
CPU: 2 PID: 11796 Comm: syz-executor5 Not tainted 4.11.0-rc6+ #211
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+0x292/0x398 lib/dump_stack.c:52
kasan_object_err+0x1c/0x70 mm/kasan/report.c:164
print_address_description mm/kasan/report.c:202 [inline]
kasan_report_error mm/kasan/report.c:291 [inline]
kasan_report+0x252/0x510 mm/kasan/report.c:347
__asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:365
parse_ipsecrequest net/key/af_key.c:1897 [inline]
parse_ipsecrequests+0xc89/0xdc0 net/key/af_key.c:1949
pfkey_compile_policy+0xa20/0xd40 net/key/af_key.c:3241
xfrm_user_policy+0x34c/0x560 net/xfrm/xfrm_state.c:1892
do_ip_setsockopt.isra.12+0x1afa/0x36c0 net/ipv4/ip_sockglue.c:1175
ip_setsockopt+0x3a/0xb0 net/ipv4/ip_sockglue.c:1264
tcp_setsockopt+0x82/0xd0 net/ipv4/tcp.c:2731
sock_common_setsockopt+0x95/0xd0 net/core/sock.c:2750
SYSC_setsockopt net/socket.c:1798 [inline]
SyS_setsockopt+0x270/0x3a0 net/socket.c:1777
entry_SYSCALL_64_fastpath+0x1f/0xc2
RIP: 0033:0x4458d9
RSP: 002b:00007f319fffdb58 EFLAGS: 00000292 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004458d9
RDX: 0000000000000010 RSI: 0000000000000000 RDI: 0000000000000005
RBP: 00000000006e3560 R08: 00000000000000e8 R09: 0000000000000000
R10: 0000000020002000 R11: 0000000000000292 R12: 0000000000708000
R13: 0000000000000000 R14: 00007f319fffe9c0 R15: 00007f319fffe700
Object at ffff88006b1e3b28, in cache kernfs_node_cache size: 152
Allocated:
PID = 1
save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:59
save_stack+0x43/0xd0 mm/kasan/kasan.c:513
set_track mm/kasan/kasan.c:525 [inline]
kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:616
kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:555
slab_post_alloc_hook mm/slab.h:456 [inline]
slab_alloc_node mm/slub.c:2718 [inline]
slab_alloc mm/slub.c:2726 [inline]
kmem_cache_alloc+0x1af/0x250 mm/slub.c:2731
kmem_cache_zalloc include/linux/slab.h:653 [inline]
__kernfs_new_node+0xd1/0x3e0 fs/kernfs/dir.c:629
kernfs_new_node+0x80/0xe0 fs/kernfs/dir.c:661
__kernfs_create_file+0x4b/0x320 fs/kernfs/file.c:989
sysfs_add_file_mode_ns+0x220/0x520 fs/sysfs/file.c:307
create_files fs/sysfs/group.c:64 [inline]
internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134
sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156
sysfs_slab_add+0x1d0/0x200 mm/slub.c:5659
__kmem_cache_create+0x428/0x4c0 mm/slub.c:4253
create_cache mm/slab_common.c:382 [inline]
kmem_cache_create+0x15d/0x240 mm/slab_common.c:467
jbd2_journal_init_revoke_caches+0x8a/0xaf fs/jbd2/revoke.c:206
journal_init_caches fs/jbd2/journal.c:2631 [inline]
journal_init+0xf/0x14d fs/jbd2/journal.c:2656
do_one_initcall+0xf3/0x390 init/main.c:792
do_initcall_level init/main.c:858 [inline]
do_initcalls init/main.c:866 [inline]
do_basic_setup init/main.c:884 [inline]
kernel_init_freeable+0x5d1/0x6a6 init/main.c:1035
kernel_init+0x13/0x180 init/main.c:959
ret_from_fork+0x31/0x40 arch/x86/entry/entry_64.S:430
Freed:
PID = 0
(stack is not available)
Memory state around the buggy address:
ffff88006b1e3c00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88006b1e3c80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88006b1e3d00: fc fc fc 00 00 00 00 00 00 00 00 00 00 00 00 00
^
ffff88006b1e3d80: 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc
ffff88006b1e3e00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
On Wed, Apr 12, 2017 at 8:02 AM, Andrey Konovalov <[email protected]> wrote:
> Hi,
>
> I've got the following error report while fuzzing the kernel with syzkaller.
>
> On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
>
> A reproducer and .config are attached.
>
> When subtracting rq->sadb_x_ipsecrequest_len from len it can become
> negative and the while loop condition remains true.
Good catch! Seems the fix is pretty straight forward:
diff --git a/net/key/af_key.c b/net/key/af_key.c
index c6252ed..cbce595 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1945,7 +1945,7 @@ parse_ipsecrequests(struct xfrm_policy *xp,
struct sadb_x_policy *pol)
if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
return -EINVAL;
- while (len >= sizeof(struct sadb_x_ipsecrequest)) {
+ while (len >= (int)sizeof(struct sadb_x_ipsecrequest)) {
if ((err = parse_ipsecrequest(xp, rq)) < 0)
return err;
len -= rq->sadb_x_ipsecrequest_len;
But pol->sadb_x_policy_len and rq->sadb_x_ipsecrequest_len
are controllable by user (fortunately root), I am feeling there might
be other problem I miss too.
On Wed, Apr 12, 2017 at 05:39:22PM -0700, Cong Wang wrote:
> On Wed, Apr 12, 2017 at 8:02 AM, Andrey Konovalov <[email protected]> wrote:
> > Hi,
> >
> > I've got the following error report while fuzzing the kernel with syzkaller.
> >
> > On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
> >
> > A reproducer and .config are attached.
> >
> > When subtracting rq->sadb_x_ipsecrequest_len from len it can become
> > negative and the while loop condition remains true.
>
> Good catch! Seems the fix is pretty straight forward:
>
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index c6252ed..cbce595 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -1945,7 +1945,7 @@ parse_ipsecrequests(struct xfrm_policy *xp,
> struct sadb_x_policy *pol)
> if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
> return -EINVAL;
>
> - while (len >= sizeof(struct sadb_x_ipsecrequest)) {
> + while (len >= (int)sizeof(struct sadb_x_ipsecrequest)) {
> if ((err = parse_ipsecrequest(xp, rq)) < 0)
> return err;
> len -= rq->sadb_x_ipsecrequest_len;
>
> But pol->sadb_x_policy_len and rq->sadb_x_ipsecrequest_len
> are controllable by user (fortunately root), I am feeling there might
> be other problem I miss too.
Well the fact that it is negative means that you've already parsed
crap in the previous loop. This interface really needs to die.
---8<---
Subject: af_key: Fix sadb_x_ipsecrequest parsing
The parsing of sadb_x_ipsecrequest is broken in a number of ways.
First of all we're not verifying sadb_x_ipsecrequest_len. This
is needed when the structure carries addresses at the end. Worse
we don't even look at the length when we parse those optional
addresses.
The migration code had similar parsing code that's better but
it also has some deficiencies. The length is overcounted first
of all as it includes the header itself. It also fails to check
the length before dereferencing the sa_family field.
This patch fixes those problems in parse_sockaddr_pair and then
uses it in parse_ipsecrequest.
Reported-by: Andrey Konovalov <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
diff --git a/net/key/af_key.c b/net/key/af_key.c
index c6252ed..6ea5751 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -65,6 +65,10 @@ struct pfkey_sock {
} dump;
};
+static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
+ xfrm_address_t *saddr, xfrm_address_t *daddr,
+ u16 *family);
+
static inline struct pfkey_sock *pfkey_sk(struct sock *sk)
{
return (struct pfkey_sock *)sk;
@@ -1913,19 +1917,14 @@ static u32 gen_reqid(struct net *net)
/* addresses present only in tunnel mode */
if (t->mode == XFRM_MODE_TUNNEL) {
- u8 *sa = (u8 *) (rq + 1);
- int family, socklen;
+ int err;
- family = pfkey_sockaddr_extract((struct sockaddr *)sa,
- &t->saddr);
- if (!family)
- return -EINVAL;
-
- socklen = pfkey_sockaddr_len(family);
- if (pfkey_sockaddr_extract((struct sockaddr *)(sa + socklen),
- &t->id.daddr) != family)
- return -EINVAL;
- t->encap_family = family;
+ err = parse_sockaddr_pair(
+ (struct sockaddr *)(rq + 1),
+ rq->sadb_x_ipsecrequest_len - sizeof(*rq),
+ &t->saddr, &t->id.daddr, &t->encap_family);
+ if (err)
+ return err;
} else
t->encap_family = xp->family;
@@ -1945,7 +1944,11 @@ static u32 gen_reqid(struct net *net)
if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
return -EINVAL;
- while (len >= sizeof(struct sadb_x_ipsecrequest)) {
+ while (len >= sizeof(*rq)) {
+ if (len < rq->sadb_x_ipsecrequest_len ||
+ rq->sadb_x_ipsecrequest_len < sizeof(*rq))
+ return -EINVAL;
+
if ((err = parse_ipsecrequest(xp, rq)) < 0)
return err;
len -= rq->sadb_x_ipsecrequest_len;
@@ -2408,7 +2411,6 @@ static int key_pol_get_resp(struct sock *sk, struct xfrm_policy *xp, const struc
return err;
}
-#ifdef CONFIG_NET_KEY_MIGRATE
static int pfkey_sockaddr_pair_size(sa_family_t family)
{
return PFKEY_ALIGN8(pfkey_sockaddr_len(family) * 2);
@@ -2420,7 +2422,7 @@ static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
{
int af, socklen;
- if (ext_len < pfkey_sockaddr_pair_size(sa->sa_family))
+ if (ext_len < 2 || ext_len < pfkey_sockaddr_pair_size(sa->sa_family))
return -EINVAL;
af = pfkey_sockaddr_extract(sa, saddr);
@@ -2436,6 +2438,7 @@ static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
return 0;
}
+#ifdef CONFIG_NET_KEY_MIGRATE
static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
struct xfrm_migrate *m)
{
@@ -2443,13 +2446,14 @@ static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
struct sadb_x_ipsecrequest *rq2;
int mode;
- if (len <= sizeof(struct sadb_x_ipsecrequest) ||
- len < rq1->sadb_x_ipsecrequest_len)
+ if (len < sizeof(*rq1) ||
+ len < rq1->sadb_x_ipsecrequest_len ||
+ rq1->sadb_x_ipsecrequest_len < sizeof(*rq1))
return -EINVAL;
/* old endoints */
err = parse_sockaddr_pair((struct sockaddr *)(rq1 + 1),
- rq1->sadb_x_ipsecrequest_len,
+ rq1->sadb_x_ipsecrequest_len - sizeof(*rq1),
&m->old_saddr, &m->old_daddr,
&m->old_family);
if (err)
@@ -2458,13 +2462,14 @@ static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
rq2 = (struct sadb_x_ipsecrequest *)((u8 *)rq1 + rq1->sadb_x_ipsecrequest_len);
len -= rq1->sadb_x_ipsecrequest_len;
- if (len <= sizeof(struct sadb_x_ipsecrequest) ||
- len < rq2->sadb_x_ipsecrequest_len)
+ if (len <= sizeof(*rq2) ||
+ len < rq2->sadb_x_ipsecrequest_len ||
+ rq2->sadb_x_ipsecrequest_len < sizeof(*rq2))
return -EINVAL;
/* new endpoints */
err = parse_sockaddr_pair((struct sockaddr *)(rq2 + 1),
- rq2->sadb_x_ipsecrequest_len,
+ rq2->sadb_x_ipsecrequest_len - sizeof(*rq2),
&m->new_saddr, &m->new_daddr,
&m->new_family);
if (err)
--
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, Apr 13, 2017 at 06:35:59PM +0800, Herbert Xu wrote:
> On Wed, Apr 12, 2017 at 05:39:22PM -0700, Cong Wang wrote:
> > On Wed, Apr 12, 2017 at 8:02 AM, Andrey Konovalov <[email protected]> wrote:
> > > Hi,
> > >
> > > I've got the following error report while fuzzing the kernel with syzkaller.
> > >
> > > On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
> > >
> > > A reproducer and .config are attached.
> > >
> > > When subtracting rq->sadb_x_ipsecrequest_len from len it can become
> > > negative and the while loop condition remains true.
> >
> > Good catch! Seems the fix is pretty straight forward:
> >
> > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > index c6252ed..cbce595 100644
> > --- a/net/key/af_key.c
> > +++ b/net/key/af_key.c
> > @@ -1945,7 +1945,7 @@ parse_ipsecrequests(struct xfrm_policy *xp,
> > struct sadb_x_policy *pol)
> > if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
> > return -EINVAL;
> >
> > - while (len >= sizeof(struct sadb_x_ipsecrequest)) {
> > + while (len >= (int)sizeof(struct sadb_x_ipsecrequest)) {
> > if ((err = parse_ipsecrequest(xp, rq)) < 0)
> > return err;
> > len -= rq->sadb_x_ipsecrequest_len;
> >
> > But pol->sadb_x_policy_len and rq->sadb_x_ipsecrequest_len
> > are controllable by user (fortunately root), I am feeling there might
> > be other problem I miss too.
>
> Well the fact that it is negative means that you've already parsed
> crap in the previous loop. This interface really needs to die.
Do you see a chance to remove this? I guess it is not used frequently
anymore, but distros still ship the old ipsec tools.
>
> ---8<---
> Subject: af_key: Fix sadb_x_ipsecrequest parsing
>
> The parsing of sadb_x_ipsecrequest is broken in a number of ways.
> First of all we're not verifying sadb_x_ipsecrequest_len. This
> is needed when the structure carries addresses at the end. Worse
> we don't even look at the length when we parse those optional
> addresses.
>
> The migration code had similar parsing code that's better but
> it also has some deficiencies. The length is overcounted first
> of all as it includes the header itself. It also fails to check
> the length before dereferencing the sa_family field.
>
> This patch fixes those problems in parse_sockaddr_pair and then
> uses it in parse_ipsecrequest.
>
> Reported-by: Andrey Konovalov <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
Applied to the ipsec tree, thanks Herbert!