* [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
@ 2017-06-07 2:08 NeilBrown
2017-06-15 23:34 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: NeilBrown @ 2017-06-07 2:08 UTC (permalink / raw)
To: Ian Kent, Andrew Morton; +Cc: LKML, autofs mailing list
[-- Attachment #1: Type: text/plain, Size: 990 bytes --]
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().
Signed-off-by: NeilBrown <neilb@suse.com>
---
fs/autofs4/dev-ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 734cbf8d9676..dd9f1bebb5a3 100644
--- 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);
}
--
2.12.2
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
2017-06-07 2:08 [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL NeilBrown
@ 2017-06-15 23:34 ` Andrew Morton
2017-06-16 2:13 ` NeilBrown
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2017-06-15 23:34 UTC (permalink / raw)
To: NeilBrown; +Cc: Ian Kent, LKML, autofs mailing list
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
2017-06-15 23:34 ` Andrew Morton
@ 2017-06-16 2:13 ` NeilBrown
2017-06-16 3:20 ` Ian Kent
2017-06-16 10:33 ` Michael Ellerman
0 siblings, 2 replies; 5+ messages in thread
From: NeilBrown @ 2017-06-16 2:13 UTC (permalink / raw)
To: Andrew Morton; +Cc: Ian Kent, LKML, autofs mailing list
[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
2017-06-16 2:13 ` NeilBrown
@ 2017-06-16 3:20 ` Ian Kent
2017-06-16 10:33 ` Michael Ellerman
1 sibling, 0 replies; 5+ messages in thread
From: Ian Kent @ 2017-06-16 3:20 UTC (permalink / raw)
To: NeilBrown, Andrew Morton; +Cc: LKML, autofs mailing list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL
2017-06-16 2:13 ` NeilBrown
2017-06-16 3:20 ` Ian Kent
@ 2017-06-16 10:33 ` Michael Ellerman
1 sibling, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2017-06-16 10:33 UTC (permalink / raw)
To: NeilBrown, Andrew Morton; +Cc: Ian Kent, LKML, autofs mailing list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-16 10:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-07 2:08 [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL NeilBrown
2017-06-15 23:34 ` Andrew Morton
2017-06-16 2:13 ` NeilBrown
2017-06-16 3:20 ` Ian Kent
2017-06-16 10:33 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).