| Message ID | ef5e609602df6d7e2b4aa07b92600f04b6851902.1512041070.git.dongsu@kinvolk.io |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [01/11] block_dev: Support checking inode permissions in lookup_bdev() | expand |
On 22/12/2017 10:32 PM, Dongsu Park wrote: > From: Seth Forshee <seth.forshee@canonical.com> > > When looking up a block device by path no permission check is > done to verify that the user has access to the block device inode > at the specified path. In some cases it may be necessary to > check permissions towards the inode, such as allowing > unprivileged users to mount block devices in user namespaces. > > Add an argument to lookup_bdev() to optionally perform this > permission check. A value of 0 skips the permission check and > behaves the same as before. A non-zero value specifies the mask > of access rights required towards the inode at the specified > path. The check is always skipped if the user has CAP_SYS_ADMIN. > > All callers of lookup_bdev() currently pass a mask of 0, so this > patch results in no functional change. Subsequent patches will > add permission checks where appropriate. > > Patch v4 is available: https://patchwork.kernel.org/patch/8943601/ > > Cc: dm-devel@redhat.com > Cc: linux-bcache@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-mtd@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Jan Kara <jack@suse.com> > Cc: Serge Hallyn <serge@hallyn.com> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Dongsu Park <dongsu@kinvolk.io> Hi Dongsu, Could you please use a macro like NO_PERMISSION_CHECK to replace hard coded 0 ? At least for me, I don't need to check what does 0 mean in the new lookup_bdev(). Thanks. Coly Li > --- > drivers/md/bcache/super.c | 2 +- > drivers/md/dm-table.c | 2 +- > drivers/mtd/mtdsuper.c | 2 +- > fs/block_dev.c | 13 ++++++++++--- > fs/quota/quota.c | 2 +- > include/linux/fs.h | 2 +- > 6 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index b4d28928..acc9d56c 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -1967,7 +1967,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, > sb); > if (IS_ERR(bdev)) { > if (bdev == ERR_PTR(-EBUSY)) { > - bdev = lookup_bdev(strim(path)); > + bdev = lookup_bdev(strim(path), 0); > mutex_lock(&bch_register_lock); > if (!IS_ERR(bdev) && bch_is_open(bdev)) > err = "device already registered"; > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 88130b5d..bca5eaf4 100644 [snip]
On Fri, Dec 22, 2017 at 03:32:25PM +0100, Dongsu Park wrote: > From: Seth Forshee <seth.forshee@canonical.com> > > When looking up a block device by path no permission check is > done to verify that the user has access to the block device inode > at the specified path. In some cases it may be necessary to > check permissions towards the inode, such as allowing > unprivileged users to mount block devices in user namespaces. > > Add an argument to lookup_bdev() to optionally perform this > permission check. A value of 0 skips the permission check and > behaves the same as before. A non-zero value specifies the mask > of access rights required towards the inode at the specified > path. The check is always skipped if the user has CAP_SYS_ADMIN. > > All callers of lookup_bdev() currently pass a mask of 0, so this > patch results in no functional change. Subsequent patches will > add permission checks where appropriate. > > Patch v4 is available: https://patchwork.kernel.org/patch/8943601/ > > Cc: dm-devel@redhat.com > Cc: linux-bcache@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: linux-mtd@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Jan Kara <jack@suse.com> > Cc: Serge Hallyn <serge@hallyn.com> Acked-by: Serge Hallyn <serge@hallyn.com> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Dongsu Park <dongsu@kinvolk.io> > --- > drivers/md/bcache/super.c | 2 +- > drivers/md/dm-table.c | 2 +- > drivers/mtd/mtdsuper.c | 2 +- > fs/block_dev.c | 13 ++++++++++--- > fs/quota/quota.c | 2 +- > include/linux/fs.h | 2 +- > 6 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index b4d28928..acc9d56c 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -1967,7 +1967,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, > sb); > if (IS_ERR(bdev)) { > if (bdev == ERR_PTR(-EBUSY)) { > - bdev = lookup_bdev(strim(path)); > + bdev = lookup_bdev(strim(path), 0); > mutex_lock(&bch_register_lock); > if (!IS_ERR(bdev) && bch_is_open(bdev)) > err = "device already registered"; > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 88130b5d..bca5eaf4 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -410,7 +410,7 @@ dev_t dm_get_dev_t(const char *path) > dev_t dev; > struct block_device *bdev; > > - bdev = lookup_bdev(path); > + bdev = lookup_bdev(path, 0); > if (IS_ERR(bdev)) > dev = name_to_dev_t(path); > else { > diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c > index e43fea89..4a4d40c0 100644 > --- a/drivers/mtd/mtdsuper.c > +++ b/drivers/mtd/mtdsuper.c > @@ -180,7 +180,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags, > /* try the old way - the hack where we allowed users to mount > * /dev/mtdblock$(n) but didn't actually _use_ the blockdev > */ > - bdev = lookup_bdev(dev_name); > + bdev = lookup_bdev(dev_name, 0); > if (IS_ERR(bdev)) { > ret = PTR_ERR(bdev); > pr_debug("MTDSB: lookup_bdev() returned %d\n", ret); > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 4a181fcb..5ca06095 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -1662,7 +1662,7 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode, > struct block_device *bdev; > int err; > > - bdev = lookup_bdev(path); > + bdev = lookup_bdev(path, 0); > if (IS_ERR(bdev)) > return bdev; > > @@ -2052,12 +2052,14 @@ EXPORT_SYMBOL(ioctl_by_bdev); > /** > * lookup_bdev - lookup a struct block_device by name > * @pathname: special file representing the block device > + * @mask: rights to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC) > * > * Get a reference to the blockdevice at @pathname in the current > * namespace if possible and return it. Return ERR_PTR(error) > - * otherwise. > + * otherwise. If @mask is non-zero, check for access rights to the > + * inode at @pathname. > */ > -struct block_device *lookup_bdev(const char *pathname) > +struct block_device *lookup_bdev(const char *pathname, int mask) > { > struct block_device *bdev; > struct inode *inode; > @@ -2072,6 +2074,11 @@ struct block_device *lookup_bdev(const char *pathname) > return ERR_PTR(error); > > inode = d_backing_inode(path.dentry); > + if (mask != 0 && !capable(CAP_SYS_ADMIN)) { > + error = __inode_permission(inode, mask); > + if (error) > + goto fail; > + } > error = -ENOTBLK; > if (!S_ISBLK(inode->i_mode)) > goto fail; > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index 43612e2a..e5d47955 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -807,7 +807,7 @@ static struct super_block *quotactl_block(const char __user *special, int cmd) > > if (IS_ERR(tmp)) > return ERR_CAST(tmp); > - bdev = lookup_bdev(tmp->name); > + bdev = lookup_bdev(tmp->name, 0); > putname(tmp); > if (IS_ERR(bdev)) > return ERR_CAST(bdev); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2995a271..fce19c49 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2551,7 +2551,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name) > #define BLKDEV_MAJOR_MAX 512 > extern const char *__bdevname(dev_t, char *buffer); > extern const char *bdevname(struct block_device *bdev, char *buffer); > -extern struct block_device *lookup_bdev(const char *); > +extern struct block_device *lookup_bdev(const char *, int mask); > extern void blkdev_show(struct seq_file *,off_t); > > #else > -- > 2.13.6
Hi, On Fri, Dec 22, 2017 at 7:59 PM, Coly Li <i@coly.li> wrote: > On 22/12/2017 10:32 PM, Dongsu Park wrote: > Hi Dongsu, > > Could you please use a macro like NO_PERMISSION_CHECK to replace hard > coded 0 ? At least for me, I don't need to check what does 0 mean in the > new lookup_bdev(). I see. I'll do that. Thanks, Dongsu > Thanks. > > Coly Li > >> --- >> drivers/md/bcache/super.c | 2 +- >> drivers/md/dm-table.c | 2 +- >> drivers/mtd/mtdsuper.c | 2 +- >> fs/block_dev.c | 13 ++++++++++--- >> fs/quota/quota.c | 2 +- >> include/linux/fs.h | 2 +- >> 6 files changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c >> index b4d28928..acc9d56c 100644 >> --- a/drivers/md/bcache/super.c >> +++ b/drivers/md/bcache/super.c >> @@ -1967,7 +1967,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, >> sb); >> if (IS_ERR(bdev)) { >> if (bdev == ERR_PTR(-EBUSY)) { >> - bdev = lookup_bdev(strim(path)); >> + bdev = lookup_bdev(strim(path), 0); >> mutex_lock(&bch_register_lock); >> if (!IS_ERR(bdev) && bch_is_open(bdev)) >> err = "device already registered"; >> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c >> index 88130b5d..bca5eaf4 100644 > [snip] > > > -- > Coly Li
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index b4d28928..acc9d56c 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -1967,7 +1967,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, sb); if (IS_ERR(bdev)) { if (bdev == ERR_PTR(-EBUSY)) { - bdev = lookup_bdev(strim(path)); + bdev = lookup_bdev(strim(path), 0); mutex_lock(&bch_register_lock); if (!IS_ERR(bdev) && bch_is_open(bdev)) err = "device already registered"; diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 88130b5d..bca5eaf4 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -410,7 +410,7 @@ dev_t dm_get_dev_t(const char *path) dev_t dev; struct block_device *bdev; - bdev = lookup_bdev(path); + bdev = lookup_bdev(path, 0); if (IS_ERR(bdev)) dev = name_to_dev_t(path); else { diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c index e43fea89..4a4d40c0 100644 --- a/drivers/mtd/mtdsuper.c +++ b/drivers/mtd/mtdsuper.c @@ -180,7 +180,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags, /* try the old way - the hack where we allowed users to mount * /dev/mtdblock$(n) but didn't actually _use_ the blockdev */ - bdev = lookup_bdev(dev_name); + bdev = lookup_bdev(dev_name, 0); if (IS_ERR(bdev)) { ret = PTR_ERR(bdev); pr_debug("MTDSB: lookup_bdev() returned %d\n", ret); diff --git a/fs/block_dev.c b/fs/block_dev.c index 4a181fcb..5ca06095 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -1662,7 +1662,7 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode, struct block_device *bdev; int err; - bdev = lookup_bdev(path); + bdev = lookup_bdev(path, 0); if (IS_ERR(bdev)) return bdev; @@ -2052,12 +2052,14 @@ EXPORT_SYMBOL(ioctl_by_bdev); /** * lookup_bdev - lookup a struct block_device by name * @pathname: special file representing the block device + * @mask: rights to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC) * * Get a reference to the blockdevice at @pathname in the current * namespace if possible and return it. Return ERR_PTR(error) - * otherwise. + * otherwise. If @mask is non-zero, check for access rights to the + * inode at @pathname. */ -struct block_device *lookup_bdev(const char *pathname) +struct block_device *lookup_bdev(const char *pathname, int mask) { struct block_device *bdev; struct inode *inode; @@ -2072,6 +2074,11 @@ struct block_device *lookup_bdev(const char *pathname) return ERR_PTR(error); inode = d_backing_inode(path.dentry); + if (mask != 0 && !capable(CAP_SYS_ADMIN)) { + error = __inode_permission(inode, mask); + if (error) + goto fail; + } error = -ENOTBLK; if (!S_ISBLK(inode->i_mode)) goto fail; diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 43612e2a..e5d47955 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -807,7 +807,7 @@ static struct super_block *quotactl_block(const char __user *special, int cmd) if (IS_ERR(tmp)) return ERR_CAST(tmp); - bdev = lookup_bdev(tmp->name); + bdev = lookup_bdev(tmp->name, 0); putname(tmp); if (IS_ERR(bdev)) return ERR_CAST(bdev); diff --git a/include/linux/fs.h b/include/linux/fs.h index 2995a271..fce19c49 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2551,7 +2551,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name) #define BLKDEV_MAJOR_MAX 512 extern const char *__bdevname(dev_t, char *buffer); extern const char *bdevname(struct block_device *bdev, char *buffer); -extern struct block_device *lookup_bdev(const char *); +extern struct block_device *lookup_bdev(const char *, int mask); extern void blkdev_show(struct seq_file *,off_t); #else