linux-kernel - Re: [PATCH 11/18] staging: gasket: always allow root open for write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE_wzQ_YosCJro46uuF1v6TQhVaTaXLdiLVD7gb94BwQx00SuQ@mail.gmail.com>
Date:   Sun, 15 Jul 2018 12:11:47 +0300
From:   Dmitry Torokhov <dtor@...omium.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     toddpoynor@...il.com, rspringer@...gle.com, jnjoseph@...gle.com,
        benchan@...omium.org, devel@...verdev.osuosl.org,
        frankhu@...omium.org, lkml <linux-kernel@...r.kernel.org>,
        Simon Que <sque@...omium.org>,
        Guenter Roeck <groeck@...omium.org>, toddpoynor@...gle.com
Subject: Re: [PATCH 11/18] staging: gasket: always allow root open for write

On Sun, Jul 15, 2018 at 12:05 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Fri, Jul 13, 2018 at 10:58:09PM -0700, Todd Poynor wrote:
> > From: Todd Poynor <toddpoynor@...gle.com>
> >
> > Always allow root to open device for writing.
> >
> > Drop special-casing of ioctl permissions for root vs. owner.
> >
> > Reported-by: Dmitry Torokhov <dtor@...omium.org>
> > Signed-off-by: Zhongze Hu <frankhu@...omium.org>
> > Signed-off-by: Todd Poynor <toddpoynor@...gle.com>
> > ---
> >  drivers/staging/gasket/apex_driver.c  |  9 +++------
> >  drivers/staging/gasket/gasket_core.c  |  8 +++++---
> >  drivers/staging/gasket/gasket_ioctl.c | 15 ++++++---------
> >  3 files changed, 14 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> > index b1318482ba65..ffe11d8168ea 100644
> > --- a/drivers/staging/gasket/apex_driver.c
> > +++ b/drivers/staging/gasket/apex_driver.c
> > @@ -644,13 +644,10 @@ static bool is_gcb_in_reset(struct gasket_dev *gasket_dev)
> >  static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> >  {
> >       struct gasket_dev *gasket_dev = filp->private_data;
> > -     int root = capable(CAP_SYS_ADMIN);
> > -     int is_owner = gasket_dev->dev_info.ownership.is_owned &&
> > -                    current->tgid == gasket_dev->dev_info.ownership.owner;
> > +     fmode_t write;
> >
> > -     if (root || is_owner)
> > -             return 1;
> > -     return 0;
> > +     write = filp->f_mode & FMODE_WRITE;
>
> Ok, this is insane.  You don't change, or check, the permissions on a
> file handle while it is already open, as you only check the permissions
> on OPEN, not on WRITE.  See the recent rant from Linus on the linux-api
> list for yet-another-long-threaad in which he explains this.
>
> So this whole ioctl can just be removed, it is totally crazy and wrong
> and should just be removed.

No, the code checks whether the requested ioctl command is compatible
with the mode the file handle was open with. There are some ioctls
that are allowed on file handle opened for read and others that
require file handle to be opened for write. That is all. It does not
change permissions past open.

Thanks,
Dmitry