linux-kernel - Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path

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] [day] [month] [year] [list]

Message-ID: <CAOQ4uxh9q_yQm9La2b3xYPqD9GPCHm8Gd+Ldi5HjVVD4NTOttA@mail.gmail.com>
Date:   Tue, 23 Jun 2020 21:59:48 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Naresh Kamboju <naresh.kamboju@...aro.org>
Cc:     Sasha Levin <sashal@...nel.org>,
        open list <linux-kernel@...r.kernel.org>,
        linux- stable <stable@...r.kernel.org>,
        Miklos Szeredi <mszeredi@...hat.com>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        lkft-triage@...ts.linaro.org
Subject: Re: [PATCH AUTOSEL 4.14 090/108] ovl: verify permissions in ovl_path_open()

On Tue, Jun 23, 2020 at 9:28 PM Naresh Kamboju
<naresh.kamboju@...aro.org> wrote:
>
> On Tue, 23 Jun 2020 at 22:46, Sasha Levin <sashal@...nel.org> wrote:
> >
> > On Tue, Jun 23, 2020 at 08:55:38PM +0530, Naresh Kamboju wrote:
> > >On Thu, 18 Jun 2020 at 07:18, Sasha Levin <sashal@...nel.org> wrote:
> > >>
> > >> From: Miklos Szeredi <mszeredi@...hat.com>
> > >>
> > >> [ Upstream commit 56230d956739b9cb1cbde439d76227d77979a04d ]
> > >>
> > >> Check permission before opening a real file.
> > >>
> > >> ovl_path_open() is used by readdir and copy-up routines.
> > >>
> > >> ovl_permission() theoretically already checked copy up permissions, but it
> > >> doesn't hurt to re-do these checks during the actual copy-up.
> > >>
> > >> For directory reading ovl_permission() only checks access to topmost
> > >> underlying layer.  Readdir on a merged directory accesses layers below the
> > >> topmost one as well.  Permission wasn't checked for these layers.
> > >>
> > >> Note: modifying ovl_permission() to perform this check would be far more
> > >> complex and hence more bug prone.  The result is less precise permissions
> > >> returned in access(2).  If this turns out to be an issue, we can revisit
> > >> this bug.
> > >>
> > >> Signed-off-by: Miklos Szeredi <mszeredi@...hat.com>
> > >> Signed-off-by: Sasha Levin <sashal@...nel.org>
> > >> ---
> > >>  fs/overlayfs/util.c | 27 ++++++++++++++++++++++++++-
> > >>  1 file changed, 26 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > >> index afdc2533ce74..76d6610767f6 100644
> > >> --- a/fs/overlayfs/util.c
> > >> +++ b/fs/overlayfs/util.c
> > >> @@ -307,7 +307,32 @@ bool ovl_is_whiteout(struct dentry *dentry)
> > >>
> > >>  struct file *ovl_path_open(struct path *path, int flags)
> > >>  {
> > >> -       return dentry_open(path, flags | O_NOATIME, current_cred());
> > >> +       struct inode *inode = d_inode(path->dentry);
> > >> +       int err, acc_mode;
> > >> +
> > >> +       if (flags & ~(O_ACCMODE | O_LARGEFILE))
> > >> +               BUG();
> > >> +
> > >> +       switch (flags & O_ACCMODE) {
> > >> +       case O_RDONLY:
> > >> +               acc_mode = MAY_READ;
> > >> +               break;
> > >> +       case O_WRONLY:
> > >> +               acc_mode = MAY_WRITE;
> > >> +               break;
> > >> +       default:
> > >> +               BUG();
> > >
> > >This BUG: triggered on stable-rc 5.7, 5.4, 4.19 and 4.14.
> > >
> > >steps to reproduce:
> > >          - cd /opt/ltp
> > >          - ./runltp -s execveat03
> >
> > Yup, that patch has been dropped, thanks for testing!
>
> After reverting this patch I see these messages while testing LTP execveat03.
> [ 87.931537] overlayfs: upper fs does not support RENAME_WHITEOUT.

This warning is new.

> [ 87.937693] overlayfs: upper fs does not support xattr, falling back
> to index=off and metacopy=off.>

This one is pretty old.

They will show up when testing with a filesystem that is suboptimal
for upper fs.

The warning is stating a fact. The suboptimal behavior with those
filesystems has been there since the beginning.

Thanks,
Amir.