Re: slaacd/iwm/loadfirmware panic: ni_pledge

[Theo de Raadt]

Stuart Henderson  wrote:

> On 2018/08/11 15:57, Theo de Raadt wrote:
> > Stuart Henderson  wrote:
> > 
> > > On 2018/08/11 19:32, Sebastien Marie wrote:
> > > >  I am also
> > > > unsure if loadfirmware() steal the slaacd context due to the use of
> > > > `curproc'.
> > > 
> > > since it was using ifconfig context (as seen with unveil) that seems 
> > > likely
> > 
> > In the ps listing there is a * next to slaacd.
> > 
> > This is not ifconfig's context.  It crashed in pledge code, because it
> > was a process which is pledged.  Which is the slaacd master.
> > 
> > It is a very tricky and fun bug.
> > 
> 
> Ah. I had been thinking slaacd might bring the interface up like ifconfig
> and dhclient do, but now I see that's not the case. Fun indeed!

I believe the kernel is implicitly bringing the interface up.  I think
ifconfig tweaks something, creating a route message.  slaacd receives
a route message, and then something happens which is running in slaacd's
context but the traceback is muddled.

Re: slaacd/iwm/loadfirmware panic: ni_pledge

[Stuart Henderson]

On 2018/08/11 15:57, Theo de Raadt wrote:
> Stuart Henderson  wrote:
> 
> > On 2018/08/11 19:32, Sebastien Marie wrote:
> > >  I am also
> > > unsure if loadfirmware() steal the slaacd context due to the use of
> > > `curproc'.
> > 
> > since it was using ifconfig context (as seen with unveil) that seems likely
> 
> In the ps listing there is a * next to slaacd.
> 
> This is not ifconfig's context.  It crashed in pledge code, because it
> was a process which is pledged.  Which is the slaacd master.
> 
> It is a very tricky and fun bug.
> 

Ah. I had been thinking slaacd might bring the interface up like ifconfig
and dhclient do, but now I see that's not the case. Fun indeed!

Re: slaacd/iwm/loadfirmware panic: ni_pledge

[Theo de Raadt]

Stuart Henderson  wrote:

> On 2018/08/11 19:32, Sebastien Marie wrote:
> >  I am also
> > unsure if loadfirmware() steal the slaacd context due to the use of
> > `curproc'.
> 
> since it was using ifconfig context (as seen with unveil) that seems likely

In the ps listing there is a * next to slaacd.

This is not ifconfig's context.  It crashed in pledge code, because it
was a process which is pledged.  Which is the slaacd master.

It is a very tricky and fun bug.

Re: slaacd/iwm/loadfirmware panic: ni_pledge

[Sebastien Marie]

On Sat, Aug 11, 2018 at 06:44:26PM +0200, Stefan Sperling wrote:
> While iwm is associated and IPv6 autoconf addresses have been
> configured by slaacd, this command can panic the kernel:
> 
>   ifconfig iwm0 down delete -inet6
> 
> It doesn't trigger every time, but after a few attempts I see:
> 
> (manually transcribed from laptop screen)
> 
> panic: ni_pledge
> Stopped at  db_enter+0x12:popq%r11
>  TID  PID UID  PRFLAGS  PFLAGS CPU COMMAND
>   515480 39389  0  0x10003   0   0 ksh
>  *364636 58348  0  0x1   0  2K slaacd
> db_enter() at db_enter()+0x12
> pledge_namei(1d8ae0144a7593e2, fff8000fff4700, 80003900) at 
> pledge_namei+0x486
> namei(d760e70ada339063) at namei+0x191
> loadfirmware(cf328604092015494,80643000,fff8006a3e78) at 
> loadfirmware+0x100
> iwm_read_firmware(fbd6fc72ee937788,0) at iwm_read_firmware+0xd7
> iwm_run_init_mvm_ucode(43bdf57cafda14a,0) at iwm_run_init_mvm_ucode+0x7f
> iwm_init_hw(ba11508ee2978b16) at iwm_init_hw+0x60
> iwm_init(3bb467f1e40db7a) at iwm_init+0x60
> iwm_ioctl(13cdfd0a249e96f,1,801ec5e00) at iwm_ioctl+0x13
> in6_ifinit(260fdd851dd74269,800643048,801ec5e00) at 
> in6_ifinit+0xab
> in6_update_ifa(28ab181e693cc4a,0,800643048) at in6_update_ifa+0xa03
> in6_ifattach_linklocal(58c794df1e3d8b3,0) at in6_ifattach_linklocal+0x1fb
> in6_ifattach(18ae40fc72b39c65) at in6_ifattach+0xcc
> end trace frame: 0x80004030
> https://www.openbsd.org describes etc. etc.
> etc. etc.
> ddb{2}>
> 

this panic is a placeholder for namei() call without proper
initialisation for ni_pledge.

here it is loadfirmware().

I am a bit unsure about how it could be trigger. Your command line show
some deinitialisation, but the origin is in6_ifattach(). I am also
unsure if loadfirmware() steal the slaacd context due to the use of
`curproc'. But as slaacd is pledged, it is required to have ni_pledge
defined.

We could try to initialise ni_pledge with PLEDGE_PATH (diff below), but
it means slaacd will need "rpath" at some point (I didn't checked if it
is currently the case or not).

Alternatively, we could also set ni_pledge with some value to say it is
an explicitely whitelisted namei() call?

For now, a diff with ni_pledge=PLEDGE_PATH. But I would prefer to
whitelist the call: this filesystem access below to the kernel and not
to slaacd.

Thanks.
-- 
Sebastien Marie


Index: dev/firmload.c
===
RCS file: /cvs/src/sys/dev/firmload.c,v
retrieving revision 1.15
diff -u -p -r1.15 firmload.c
--- dev/firmload.c  5 Aug 2018 23:19:49 -   1.15
+++ dev/firmload.c  11 Aug 2018 17:25:08 -
@@ -51,6 +51,7 @@ loadfirmware(const char *name, u_char **
}
 
NDINIT(, LOOKUP, NOFOLLOW|LOCKLEAF, UIO_SYSSPACE, path, p);
+   nid.ni_pledge = PLEDGE_RPATH;
nid.ni_cnd.cn_flags |= BYPASSUNVEIL;
error = namei();
 #ifdef RAMDISK_HOOKS