Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1667339 > unrolled thread

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

Started byAndrew Morton <akpm@linux-foundation.org>
First post2017-06-16 01:40 +0200
Last post2017-06-16 12:40 +0200
Articles 4 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] autofs: sanity check status reported with  AUTOFS_DEV_IOCTL_FAIL Andrew Morton <akpm@linux-foundation.org> - 2017-06-16 01:40 +0200
    Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL NeilBrown <neilb@suse.com> - 2017-06-16 04:20 +0200
      Re: [PATCH] autofs: sanity check status reported with  AUTOFS_DEV_IOCTL_FAIL Ian Kent <raven@themaw.net> - 2017-06-16 05:40 +0200
      Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL Michael Ellerman <mpe@ellerman.id.au> - 2017-06-16 12:40 +0200

#1667339 — Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

FromAndrew Morton <akpm@linux-foundation.org>
Date2017-06-16 01:40 +0200
SubjectRe: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
Message-ID<tSMwO-xz-13@gated-at.bofh.it>
On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown <neilb@suse.com> wrote:

> 
> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> ioctl, autofs4_d_automount() will return
>    ERR_PTR(status)
> with that status to follow_automount(), which will then
> dereference an invalid pointer.
> 
> So treat a positive status the same as zero, and map
> to ENOENT.
> 
> See comment in systemd src/core/automount.c::automount_send_ready().
> 
> ...
>
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>  	int status;
>  
>  	token = (autofs_wqt_t) param->fail.token;
> -	status = param->fail.status ? param->fail.status : -ENOENT;
> +	status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>  	return autofs4_wait_release(sbi, token, status);
>  }

Sounds serious.  Was the absence of a cc:stable deliberate?

[toc] | [next] | [standalone]

#1667381 — Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

FromNeilBrown <neilb@suse.com>
Date2017-06-16 04:20 +0200
SubjectRe: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
Message-ID<tSP1D-2jZ-1@gated-at.bofh.it>
In reply to#1667339

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jun 15 2017, Andrew Morton wrote:

> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown <neilb@suse.com> wrote:
>
>> 
>> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
>> ioctl, autofs4_d_automount() will return
>>    ERR_PTR(status)
>> with that status to follow_automount(), which will then
>> dereference an invalid pointer.
>> 
>> So treat a positive status the same as zero, and map
>> to ENOENT.
>> 
>> See comment in systemd src/core/automount.c::automount_send_ready().
>> 
>> ...
>>
>> --- a/fs/autofs4/dev-ioctl.c
>> +++ b/fs/autofs4/dev-ioctl.c
>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>  	int status;
>>  
>>  	token = (autofs_wqt_t) param->fail.token;
>> -	status = param->fail.status ? param->fail.status : -ENOENT;
>> +	status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>  	return autofs4_wait_release(sbi, token, status);
>>  }
>
> Sounds serious.  Was the absence of a cc:stable deliberate?

You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
mean the bug can only be triggered by a process that could easily do
worse?

Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
people??  I haven't been keeping up.

Given how simple the patch is, it probably makes sense to add a
cc:stable, just in case.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1667401

FromIan Kent <raven@themaw.net>
Date2017-06-16 05:40 +0200
Message-ID<tSQh3-32M-5@gated-at.bofh.it>
In reply to#1667381
On Fri, 2017-06-16 at 12:13 +1000, NeilBrown wrote:
> On Thu, Jun 15 2017, Andrew Morton wrote:
> 
> > On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown <neilb@suse.com> wrote:
> > 
> > > 
> > > If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> > > ioctl, autofs4_d_automount() will return
> > >    ERR_PTR(status)
> > > with that status to follow_automount(), which will then
> > > dereference an invalid pointer.
> > > 
> > > So treat a positive status the same as zero, and map
> > > to ENOENT.
> > > 
> > > See comment in systemd src/core/automount.c::automount_send_ready().
> > > 
> > > ...
> > > 
> > > --- a/fs/autofs4/dev-ioctl.c
> > > +++ b/fs/autofs4/dev-ioctl.c
> > > @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
> > >  	int status;
> > >  
> > >  	token = (autofs_wqt_t) param->fail.token;
> > > -	status = param->fail.status ? param->fail.status : -ENOENT;
> > > +	status = param->fail.status < 0 ? param->fail.status : -ENOENT;
> > >  	return autofs4_wait_release(sbi, token, status);
> > >  }
> > 
> > Sounds serious.  Was the absence of a cc:stable deliberate?
> 
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?

Think so, yes.

> 
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Maybe, with docker root can start a container with --privileged to give the
container admin capabilities. It may be the case that capabilities can be used
now I don't know.

> 
> Given how simple the patch is, it probably makes sense to add a
> cc:stable, just in case.

IMHO it needs to be applied to stable as well.

> 
> Thanks,
> NeilBrown

[toc] | [prev] | [next] | [standalone]

#1667625 — Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-06-16 12:40 +0200
SubjectRe: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
Message-ID<tSWPw-7fv-1@gated-at.bofh.it>
In reply to#1667381
NeilBrown <neilb@suse.com> writes:
> On Thu, Jun 15 2017, Andrew Morton wrote:
>> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown <neilb@suse.com> wrote:
>>> --- a/fs/autofs4/dev-ioctl.c
>>> +++ b/fs/autofs4/dev-ioctl.c
>>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>>  	int status;
>>>  
>>>  	token = (autofs_wqt_t) param->fail.token;
>>> -	status = param->fail.status ? param->fail.status : -ENOENT;
>>> +	status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>>  	return autofs4_wait_release(sbi, token, status);
>>>  }
>>
>> Sounds serious.  Was the absence of a cc:stable deliberate?
>
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?
>
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Yep. They can be configured individually in fact, eg:

  $ docker run --cap-drop=ALL --cap-add=sys_admin -it debian:jessie /bin/bash
  root@aedebe8c46e0:/# capsh --print
  Current: = cap_sys_admin+eip
  Bounding set =cap_sys_admin
  Securebits: 00/0x0/1'b0
   secure-noroot: no (unlocked)
   secure-no-suid-fixup: no (unlocked)
   secure-keep-caps: no (unlocked)
  uid=0(root)
  gid=0(root)
  groups=


cheers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web