Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

From: Miklos Szeredi
Date: Thu Mar 17 2016 - 11:21:55 EST

Next message: Javier Martinez Canillas: "Re: [RFC/RFT PATCH 0/2] mfd: max14577: Allow the driver to be built as a module"
Previous message: Srinivas Pandruvada: "Re: [PATCH v3] acpi: Issue _OSC call for native thermal interrupt handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 29, 2016 at 05:54:40PM +0100, Ignacy GawÄdzki wrote:
> On Mon, Feb 29, 2016 at 11:25:46AM -0500, thus spake Vivek Goyal:
> > I agree that semantics should be more consistent. I don't know that
> > if upper layer should override lower layer checks or not.
> >
> > One could also argue that if root did chown, then changes effectively
> > happened in upper layer and anything in upper layer should become
> > visible to unpriviliged user but not the one in lower layer.
> >
> > I just don't know. I guess those who have more background on this
> > could pitch in and clarify that was is supposed to be the design
> > intention.

I haven't thought about this aspect yet.

[thinking]

We cannot actually respect lower layer search permissions in a merged directory,
because we cannot selectively allow or deny directory search permissions on
individual directory entries (there's simply not enough information in the
->permission() call).

The only sane thing to do is to override search permission by the upper layer
permissions.

We can do that with the cap_raise(... CAP_DAC_OVERRIDE) thing, which is what
overlayfs usually does when it needs elevated permissions to do something.

Or we can a special lookup_one_len() type helper. But we already have the
dentry hash in the overlay dentry, so no need to recalculate that (we've also
made sure that underlying fs doesnt have ->d_hash()). The sanity checks on
'name' are also unnecessary. And that leaves us with __lookup_hash() where we
can just get rid of the underscores and export to modules.

Attaching folded patch. Will post split out ones for review.

Thanks,
Miklos

diff --git a/fs/namei.c b/fs/namei.c
index 9c590e0f66e9..80fe2a68f361 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1447,7 +1447,7 @@ static int follow_dotdot(struct nameidata *nd)
*
* dir->d_inode->i_mutex must be held
*/
-static struct dentry *lookup_dcache(struct qstr *name, struct dentry *dir,
+static struct dentry *lookup_dcache(const struct qstr *name, struct dentry *dir,
unsigned int flags, bool *need_lookup)
{
struct dentry *dentry;
@@ -1506,8 +1506,8 @@ static struct dentry *lookup_real(struct inode *dir, struct dentry *dentry,
return dentry;
}

-static struct dentry *__lookup_hash(struct qstr *name,
- struct dentry *base, unsigned int flags)
+struct dentry *lookup_hash(const struct qstr *name,
+ struct dentry *base, unsigned int flags)
{
bool need_lookup;
struct dentry *dentry;
@@ -1518,6 +1518,7 @@ static struct dentry *__lookup_hash(struct qstr *name,

return lookup_real(base->d_inode, dentry, flags);
}
+EXPORT_SYMBOL(lookup_hash);

/*
* It's more convoluted than I'd like it to be, but... it's still fairly
@@ -1630,7 +1631,7 @@ static int lookup_slow(struct nameidata *nd, struct path *path)
BUG_ON(nd->inode != parent->d_inode);

inode_lock(parent->d_inode);
- dentry = __lookup_hash(&nd->last, parent, nd->flags);
+ dentry = lookup_hash(&nd->last, parent, nd->flags);
inode_unlock(parent->d_inode);
if (IS_ERR(dentry))
return PTR_ERR(dentry);
@@ -2235,7 +2236,7 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
return ERR_PTR(-EINVAL);
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
- d = __lookup_hash(&last, path->dentry, 0);
+ d = lookup_hash(&last, path->dentry, 0);
if (IS_ERR(d)) {
inode_unlock(path->dentry->d_inode);
path_put(path);
@@ -2319,7 +2320,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
if (err)
return ERR_PTR(err);

- return __lookup_hash(&this, base, 0);
+ return lookup_hash(&this, base, 0);
}
EXPORT_SYMBOL(lookup_one_len);

@@ -2386,7 +2387,7 @@ struct dentry *lookup_one_len_unlocked(const char *name,
return ret;

inode_lock(base->d_inode);
- ret = __lookup_hash(&this, base, 0);
+ ret = lookup_hash(&this, base, 0);
inode_unlock(base->d_inode);
return ret;
}
@@ -3498,7 +3499,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
*/
lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL;
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
- dentry = __lookup_hash(&last, path->dentry, lookup_flags);
+ dentry = lookup_hash(&last, path->dentry, lookup_flags);
if (IS_ERR(dentry))
goto unlock;

@@ -3803,7 +3804,7 @@ retry:
goto exit1;

inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
- dentry = __lookup_hash(&last, path.dentry, lookup_flags);
+ dentry = lookup_hash(&last, path.dentry, lookup_flags);
error = PTR_ERR(dentry);
if (IS_ERR(dentry))
goto exit2;
@@ -3925,7 +3926,7 @@ retry:
goto exit1;
retry_deleg:
inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
- dentry = __lookup_hash(&last, path.dentry, lookup_flags);
+ dentry = lookup_hash(&last, path.dentry, lookup_flags);
error = PTR_ERR(dentry);
if (!IS_ERR(dentry)) {
/* Why not before? Because we want correct error value */
@@ -4443,7 +4444,7 @@ retry:
retry_deleg:
trap = lock_rename(new_path.dentry, old_path.dentry);

- old_dentry = __lookup_hash(&old_last, old_path.dentry, lookup_flags);
+ old_dentry = lookup_hash(&old_last, old_path.dentry, lookup_flags);
error = PTR_ERR(old_dentry);
if (IS_ERR(old_dentry))
goto exit3;
@@ -4451,7 +4452,7 @@ retry_deleg:
error = -ENOENT;
if (d_is_negative(old_dentry))
goto exit4;
- new_dentry = __lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags);
+ new_dentry = lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags);
error = PTR_ERR(new_dentry);
if (IS_ERR(new_dentry))
goto exit4;
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d894e7cd9a86..0cf45a0e904e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -220,8 +220,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
if (IS_ERR(newdentry))
goto out;

- upper = lookup_one_len(dentry->d_name.name, upperdir,
- dentry->d_name.len);
+ upper = lookup_hash(&dentry->d_name, upperdir, 0);
err = PTR_ERR(upper);
if (IS_ERR(upper))
goto out1;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 52f6de5d40a9..888b5fe29fdf 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -168,8 +168,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
int err;

inode_lock_nested(udir, I_MUTEX_PARENT);
- newdentry = lookup_one_len(dentry->d_name.name, upperdir,
- dentry->d_name.len);
+ newdentry = lookup_hash(&dentry->d_name, upperdir, 0);
err = PTR_ERR(newdentry);
if (IS_ERR(newdentry))
goto out_unlock;
@@ -337,8 +336,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
if (IS_ERR(newdentry))
goto out_unlock;

- upper = lookup_one_len(dentry->d_name.name, upperdir,
- dentry->d_name.len);
+ upper = lookup_hash(&dentry->d_name, upperdir, 0);
err = PTR_ERR(upper);
if (IS_ERR(upper))
goto out_dput;
@@ -547,8 +545,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)

upper = ovl_dentry_upper(dentry);
if (!upper) {
- upper = lookup_one_len(dentry->d_name.name, upperdir,
- dentry->d_name.len);
+ upper = lookup_hash(&dentry->d_name, upperdir, 0);
err = PTR_ERR(upper);
if (IS_ERR(upper))
goto kill_whiteout;
@@ -851,8 +848,7 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old,
}
} else {
new_create = true;
- newdentry = lookup_one_len(new->d_name.name, new_upperdir,
- new->d_name.len);
+ newdentry = lookup_hash(&new->d_name, new_upperdir, 0);
err = PTR_ERR(newdentry);
if (IS_ERR(newdentry))
goto out_unlock;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 619ad4b016d2..c16eeb3e5021 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -379,7 +379,7 @@ static inline struct dentry *ovl_lookup_real(struct dentry *dir,
struct dentry *dentry;

inode_lock(dir->d_inode);
- dentry = lookup_one_len(name->name, dir, name->len);
+ dentry = lookup_hash(name, dir, 0);
inode_unlock(dir->d_inode);

if (IS_ERR(dentry)) {
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d0f25d81b46a..22cc2cece000 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -78,6 +78,8 @@ extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);

extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
+struct qstr;
+extern struct dentry *lookup_hash(const struct qstr *, struct dentry *, unsigned int);

extern int follow_down_one(struct path *);
extern int follow_down(struct path *);

Next message: Javier Martinez Canillas: "Re: [RFC/RFT PATCH 0/2] mfd: max14577: Allow the driver to be built as a module"
Previous message: Srinivas Pandruvada: "Re: [PATCH v3] acpi: Issue _OSC call for native thermal interrupt handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]