On Thu, 28 Apr 2022 at 12:30, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Thu, Apr 28, 2022 at 12:10:24PM +0200, Miklos Szeredi wrote:
> > On Thu, 7 Apr 2022 at 13:23, Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > >
> > > Make the two places where lookup helpers can be called either on lower
> > > or upper layers take the mount's idmapping into account. To this end we
> > > pass down the mount in struct ovl_lookup_data. It can later also be used
> > > to construct struct path for various other helpers. This is needed to
> > > support idmapped base layers with overlay.
> > >
> > > Cc: <linux-unionfs@xxxxxxxxxxxxxxx>
> > > Tested-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
> > > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > > ---
> > > /* v2 */
> > > unchanged
> > >
> > > /* v3 */
> > > unchanged
> > >
> > > /* v4 */
> > > - Vivek Goyal <vgoyal@xxxxxxxxxx>:
> > > - s/ovl_upper_idmap()/ovl_upper_mnt_userns()/g
> > >
> > > /* v5 */
> > > unchanged
> > > ---
> > > fs/overlayfs/export.c | 3 ++-
> > > fs/overlayfs/namei.c | 14 ++++++++------
> > > fs/overlayfs/readdir.c | 10 +++++-----
> > > 3 files changed, 15 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index ebde05c9cf62..5acf353d160b 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -391,7 +391,8 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > > * pointer because we hold no lock on the real dentry.
> > > */
> > > take_dentry_name_snapshot(&name, real);
> > > - this = lookup_one_len(name.name.name, connected, name.name.len);
> > > + this = lookup_one(mnt_user_ns(layer->mnt), name.name.name,
> > > + connected, name.name.len);
> >
> > This one is tricky. It's doing a lookup on overlay, so messing with
> > the underlying mnt_userns is definitely wrong.
> >
> > Is the mnt_userns needed for permission checking? Possibly in that
> > case the permission checking needs to be skipped altogether, since
> > it's doing an fh -> dentry lookup which should succeed regardless of
> > caller's caps.
>
> If capabilities are checked then it's always caller's user namespace
> that is used (Ofc, exceptions being filesystem-wide operations where
> capabilities in the filesystem's userns are needed but that doesn't
> apply here.).
>
> Nothing has changed with the introduction of the mnt_userns in the
> area of capability checking. IOW, the mnt_userns isn't used for
> capability checks as that would be a major permission model change
> overall (It might have applications in the future ofc.).
>
> The mnt_userns matters for permission checking only in so far as it is
> used to map the k{g,u}ids according to the mount and so is relevant in
> only that sense in inode_permission().
Sorry, sloppy phrasing on my part. Yes, I meant permission checking.
> If this is doing a lookup on an overlay and the relevant mount isn't
> supposed to be idmapped then the simple thing to do is to pass
> &init_user_ns.
>
> Could you explain what "lookup on overlay" means here, i.e. what mount
> is relevant?
It's doing an fh -> dentry transform, the mount should be irrelevant.
Lookup permission cannot properly be checked, hence the operation
needs CAP_DAC_READ_SEARCH when invoked from userspace via
open_by_handle_at(2).
So I guess the proper fix would be to introduce a version of
lookup_one_len() without inode_permission()...
Thanks,
Miklos
