Re: WARNING in xfrm_state_fini
On Mon, Nov 27, 2017 at 09:37:07AM -0800, Cong Wang wrote:
> On Mon, Nov 27, 2017 at 3:55 AM, Steffen Klassert
> wrote:
> > On Tue, Nov 21, 2017 at 06:44:04PM -0800, Cong Wang wrote:
> >> User-space uses proto==0 as a wildcard, but xfrm_id_proto_match()
> >> doesn't consider it as a match with IPSEC_PROTO_ANY, in this case
> >> it should match all. Not sure if the following patch is the best way to
> >> fix it, or perhaps x->id.proto should be initialized to some of these 3
> >> values, but looking into ->init_temprop() it is not the case.
> >
> > x->id is copied from the policy template and it seems that we don't
> > validate the id of the template when inserting the policy. iproute2
> > checks for a valid IPsec proto but the kernel does not do so. I think
> > we should check the policy template and reject inserting if the proto
> > is invalid.
> >
>
> Oh, I thought 0 is used as wildcard, so it is not.
>
> Something like below?
>
> @@ -1445,6 +1446,15 @@ static int validate_tmpl(int nr, struct
> xfrm_user_tmpl *ut, u16 family)
> default:
> return -EINVAL;
> }
> + switch (ut[i].id.proto) {
> + case IPPROTO_AH:
> + case IPPROTO_ESP:
> + case IPPROTO_COMP:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> }
>
> return 0;
>
I assume this is supposed to be fixed by the following, so marking it closed for
syzbot:
#syz fix: xfrm: check id proto in validate_tmpl()
But syzbot has been hitting a WARN_ON() in xfrm_state_fini() even after that
fix, so it should get reported as a new bug.
Re: WARNING in xfrm_state_fini
On Mon, Nov 27, 2017 at 3:55 AM, Steffen Klassert
wrote:
> On Tue, Nov 21, 2017 at 06:44:04PM -0800, Cong Wang wrote:
>> User-space uses proto==0 as a wildcard, but xfrm_id_proto_match()
>> doesn't consider it as a match with IPSEC_PROTO_ANY, in this case
>> it should match all. Not sure if the following patch is the best way to
>> fix it, or perhaps x->id.proto should be initialized to some of these 3
>> values, but looking into ->init_temprop() it is not the case.
>
> x->id is copied from the policy template and it seems that we don't
> validate the id of the template when inserting the policy. iproute2
> checks for a valid IPsec proto but the kernel does not do so. I think
> we should check the policy template and reject inserting if the proto
> is invalid.
>
Oh, I thought 0 is used as wildcard, so it is not.
Something like below?
@@ -1445,6 +1446,15 @@ static int validate_tmpl(int nr, struct
xfrm_user_tmpl *ut, u16 family)
default:
return -EINVAL;
}
+ switch (ut[i].id.proto) {
+ case IPPROTO_AH:
+ case IPPROTO_ESP:
+ case IPPROTO_COMP:
+ break;
+ default:
+ return -EINVAL;
+ }
+
}
return 0;
Re: WARNING in xfrm_state_fini
On Tue, Nov 21, 2017 at 06:44:04PM -0800, Cong Wang wrote: > On Tue, Nov 21, 2017 at 2:00 AM, syzbot > > wrote: > > Hello, > > > > syzkaller hit the following crash on > > c8a0739b185d11d6e2ca7ad9f5835841d1cfc765 > > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.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 > > > > > > Kernel panic - not syncing: panic_on_warn set ... > > > > CPU: 0 PID: 21 Comm: kworker/u4:1 Not tainted 4.14.0+ #187 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > Workqueue: netns cleanup_net > > Call Trace: > > __dump_stack lib/dump_stack.c:17 [inline] > > dump_stack+0x194/0x257 lib/dump_stack.c:53 > > panic+0x1e4/0x41c kernel/panic.c:183 > > __warn+0x1dc/0x200 kernel/panic.c:547 > > report_bug+0x211/0x2d0 lib/bug.c:184 > > fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:177 > > fixup_bug arch/x86/kernel/traps.c:246 [inline] > > do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:295 > > do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:314 > > invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:926 > > RIP: 0010:xfrm_state_fini+0x46a/0x620 net/xfrm/xfrm_state.c:2323 > > RSP: 0018:8801d9ce70f0 EFLAGS: 00010293 > > RAX: 8801d9cde580 RBX: 8801ccf50040 RCX: 845cb0fa > > RDX: RSI: 11003b39bdd1 RDI: ed003b39ce10 > > RBP: 8801d9ce7248 R08: 11003b39cda4 R09: > > R10: R11: R12: 11003b39ce20 > > R13: 8801d9ce7220 R14: 11003b39ce24 R15: 8801ccf51500 > > xfrm_net_exit+0x25/0x30 net/xfrm/xfrm_policy.c:2957 > > User-space uses proto==0 as a wildcard, but xfrm_id_proto_match() > doesn't consider it as a match with IPSEC_PROTO_ANY, in this case > it should match all. Not sure if the following patch is the best way to > fix it, or perhaps x->id.proto should be initialized to some of these 3 > values, but looking into ->init_temprop() it is not the case. x->id is copied from the policy template and it seems that we don't validate the id of the template when inserting the policy. iproute2 checks for a valid IPsec proto but the kernel does not do so. I think we should check the policy template and reject inserting if the proto is invalid.
Re: WARNING in xfrm_state_fini
On Tue, Nov 21, 2017 at 2:00 AM, syzbot
wrote:
> Hello,
>
> syzkaller hit the following crash on
> c8a0739b185d11d6e2ca7ad9f5835841d1cfc765
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.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
>
>
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 0 PID: 21 Comm: kworker/u4:1 Not tainted 4.14.0+ #187
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: netns cleanup_net
> Call Trace:
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x257 lib/dump_stack.c:53
> panic+0x1e4/0x41c kernel/panic.c:183
> __warn+0x1dc/0x200 kernel/panic.c:547
> report_bug+0x211/0x2d0 lib/bug.c:184
> fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:177
> fixup_bug arch/x86/kernel/traps.c:246 [inline]
> do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:295
> do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:314
> invalid_op+0x18/0x20 arch/x86/entry/entry_64.S:926
> RIP: 0010:xfrm_state_fini+0x46a/0x620 net/xfrm/xfrm_state.c:2323
> RSP: 0018:8801d9ce70f0 EFLAGS: 00010293
> RAX: 8801d9cde580 RBX: 8801ccf50040 RCX: 845cb0fa
> RDX: RSI: 11003b39bdd1 RDI: ed003b39ce10
> RBP: 8801d9ce7248 R08: 11003b39cda4 R09:
> R10: R11: R12: 11003b39ce20
> R13: 8801d9ce7220 R14: 11003b39ce24 R15: 8801ccf51500
> xfrm_net_exit+0x25/0x30 net/xfrm/xfrm_policy.c:2957
User-space uses proto==0 as a wildcard, but xfrm_id_proto_match()
doesn't consider it as a match with IPSEC_PROTO_ANY, in this case
it should match all. Not sure if the following patch is the best way to
fix it, or perhaps x->id.proto should be initialized to some of these 3
values, but looking into ->init_temprop() it is not the case.
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index dc28a98ce97c..8752f8b2c57f 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1370,7 +1370,8 @@ static inline int xfrm_state_kern(const struct
xfrm_state *x)
static inline int xfrm_id_proto_match(u8 proto, u8 userproto)
{
return (!userproto || proto == userproto ||
- (userproto == IPSEC_PROTO_ANY && (proto == IPPROTO_AH ||
+ (userproto == IPSEC_PROTO_ANY && (!proto ||
+ proto == IPPROTO_AH ||
proto == IPPROTO_ESP ||
proto == IPPROTO_COMP)));
}
