diff mbox

[v4,3/7] fs: Verify access of user towards block device file when mounting

Message ID 1443039368-55445-4-git-send-email-seth.forshee@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

Seth Forshee Sept. 23, 2015, 8:16 p.m. UTC
When mounting a filesystem on a block device there is currently
no verification that the user has appropriate access to the
device file passed to mount. This has not been an issue so far
since the user in question has always been root, but this must
be changed before allowing unprivileged users to mount in user
namespaces.

To do this, a new version of lookup_bdev() is added named
lookup_bdev_perm(). Both of these functions become wrappers
around a common inner fucntion. The behavior of lookup_bdev() is
unchanged, but calling lookup_bdev_perm() will fail if the user
does not have the specified access rights to the supplied path.
The permission check is skipped if the user has CAP_SYS_ADMIN to
avoid any possible regressions in behavior.

blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
is used by mount_bdev() and mount_mtd(), so this will cause
mounts on block devices to fail when the user lacks the required
permissions. Other calls to blkdev_get_by_path() will all happen
with root privileges, so these calls will be unaffected.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/mtd/mtdsuper.c |  7 ++++++-
 fs/block_dev.c         | 52 ++++++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h     |  1 +
 3 files changed, 49 insertions(+), 11 deletions(-)

Comments

Eric W. Biederman Sept. 24, 2015, 9:53 p.m. UTC | #1
Seth Forshee <seth.forshee@canonical.com> writes:

> When mounting a filesystem on a block device there is currently
> no verification that the user has appropriate access to the
> device file passed to mount. This has not been an issue so far
> since the user in question has always been root, but this must
> be changed before allowing unprivileged users to mount in user
> namespaces.
>
> To do this, a new version of lookup_bdev() is added named
> lookup_bdev_perm(). Both of these functions become wrappers
> around a common inner fucntion. The behavior of lookup_bdev() is
> unchanged, but calling lookup_bdev_perm() will fail if the user
> does not have the specified access rights to the supplied path.
> The permission check is skipped if the user has CAP_SYS_ADMIN to
> avoid any possible regressions in behavior.
>
> blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
> is used by mount_bdev() and mount_mtd(), so this will cause
> mounts on block devices to fail when the user lacks the required
> permissions. Other calls to blkdev_get_by_path() will all happen
> with root privileges, so these calls will be unaffected.

Good but buggy patch.

In the mtd bits the flags are super flags, not file mode bits,
which makes testing them against FMODE_READ and FMODE_WRITE is
incorrect.

Further it looks like quite a few more possibly all of the lookup_bdev
instances could use inode level permission checking.

Certainly code such as quotactl makes me wonder.

Ugh.  Your code needs to be using __inode_permission and not
inode_permission.

The readability or writability of a device node has little or nothing to
do with the readability/writability of the superblock.   So I don't see
any reason we should be checking that.

Eric
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/mtd/mtdsuper.c |  7 ++++++-
>  fs/block_dev.c         | 52 ++++++++++++++++++++++++++++++++++++++++----------
>  include/linux/fs.h     |  1 +
>  3 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index 20c02a3b7417..a3970b0506b9 100644
> --- a/drivers/mtd/mtdsuper.c
> +++ b/drivers/mtd/mtdsuper.c
> @@ -125,6 +125,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
>  #ifdef CONFIG_BLOCK
>  	struct block_device *bdev;
>  	int ret, major;
> +	int perm = 0;
>  #endif
>  	int mtdnr;
>  
> @@ -176,7 +177,11 @@ 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);
> +	if (flags & FMODE_READ)
> +		perm |= MAY_READ;
> +	if (flags & FMODE_WRITE)
> +		perm |= MAY_WRITE;
> +	bdev = lookup_bdev_perm(dev_name, perm);
>  	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 46bd98482f71..7ca0f8911e97 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1401,9 +1401,14 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
>  					void *holder)
>  {
>  	struct block_device *bdev;
> +	int perm = 0;
>  	int err;
>  
> -	bdev = lookup_bdev(path);
> +	if (mode & FMODE_READ)
> +		perm |= MAY_READ;
> +	if (mode & FMODE_WRITE)
> +		perm |= MAY_WRITE;
> +	bdev = lookup_bdev_perm(path, perm);
>  	if (IS_ERR(bdev))
>  		return bdev;
>  
> @@ -1710,15 +1715,8 @@ int ioctl_by_bdev(struct block_device *bdev, unsigned cmd, unsigned long arg)
>  
>  EXPORT_SYMBOL(ioctl_by_bdev);
>  
> -/**
> - * lookup_bdev  - lookup a struct block_device by name
> - * @pathname:	special file representing the block device
> - *
> - * Get a reference to the blockdevice at @pathname in the current
> - * namespace if possible and return it.  Return ERR_PTR(error)
> - * otherwise.
> - */
> -struct block_device *lookup_bdev(const char *pathname)
> +static struct block_device *__lookup_bdev(const char *pathname, int mask,
> +					  bool check_perm)
>  {
>  	struct block_device *bdev;
>  	struct inode *inode;
> @@ -1733,6 +1731,11 @@ struct block_device *lookup_bdev(const char *pathname)
>  		return ERR_PTR(error);
>  
>  	inode = d_backing_inode(path.dentry);
> +	if (check_perm && !capable(CAP_SYS_ADMIN)) {
> +		error = inode_permission(inode, mask);
> +		if (error)
> +			goto fail;
> +	}
>  	error = -ENOTBLK;
>  	if (!S_ISBLK(inode->i_mode))
>  		goto fail;
> @@ -1750,6 +1753,35 @@ fail:
>  	bdev = ERR_PTR(error);
>  	goto out;
>  }
> +
> +/*
> + * lookup_bdev_perm - lookup a struct block_device by name and check for
> + *		      access rights
> + * @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, check for the specified rights to the device
> + * at @pathname, and return it.  Return ERR_PTR(error) otherwise.
> + */
> +struct block_device *lookup_bdev_perm(const char *pathname, int mask)
> +{
> +	return __lookup_bdev(pathname, mask, true);
> +}
> +EXPORT_SYMBOL(lookup_bdev_perm);
> +
> +/**
> + * lookup_bdev  - lookup a struct block_device by name
> + * @pathname:	special file representing the block device
> + *
> + * Get a reference to the blockdevice at @pathname in the current
> + * namespace if possible and return it.  Return ERR_PTR(error)
> + * otherwise.
> + */
> +struct block_device *lookup_bdev(const char *pathname)
> +{
> +	return __lookup_bdev(pathname, 0, false);
> +}
>  EXPORT_SYMBOL(lookup_bdev);
>  
>  int __invalidate_device(struct block_device *bdev, bool kill_dirty)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 5ec201e8308c..d03c2061bdd2 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2383,6 +2383,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name)
>  #define BLKDEV_MAJOR_HASH_SIZE	255
>  extern const char *__bdevname(dev_t, char *buffer);
>  extern const char *bdevname(struct block_device *bdev, char *buffer);
> +extern struct block_device *lookup_bdev_perm(const char *, int);
>  extern struct block_device *lookup_bdev(const char *);
>  extern void blkdev_show(struct seq_file *,off_t);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Sept. 25, 2015, 12:48 p.m. UTC | #2
On Thu, Sep 24, 2015 at 04:53:11PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > When mounting a filesystem on a block device there is currently
> > no verification that the user has appropriate access to the
> > device file passed to mount. This has not been an issue so far
> > since the user in question has always been root, but this must
> > be changed before allowing unprivileged users to mount in user
> > namespaces.
> >
> > To do this, a new version of lookup_bdev() is added named
> > lookup_bdev_perm(). Both of these functions become wrappers
> > around a common inner fucntion. The behavior of lookup_bdev() is
> > unchanged, but calling lookup_bdev_perm() will fail if the user
> > does not have the specified access rights to the supplied path.
> > The permission check is skipped if the user has CAP_SYS_ADMIN to
> > avoid any possible regressions in behavior.
> >
> > blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
> > is used by mount_bdev() and mount_mtd(), so this will cause
> > mounts on block devices to fail when the user lacks the required
> > permissions. Other calls to blkdev_get_by_path() will all happen
> > with root privileges, so these calls will be unaffected.
> 
> Good but buggy patch.
> 
> In the mtd bits the flags are super flags, not file mode bits,
> which makes testing them against FMODE_READ and FMODE_WRITE is
> incorrect.

Bah, yes. Fixed.

> Further it looks like quite a few more possibly all of the lookup_bdev
> instances could use inode level permission checking.
> 
> Certainly code such as quotactl makes me wonder.

I opted to stick to places related to mounting, but let's take a look at
the other callers.

bcache calls it in the context of sysfs writes, and those attributes are
writable only by root. In that case the inode permission check will be
skipped anyway, so it makes no difference either way.

Device mapper calls it in dm_get_device, which is called from a bunch of
places. I had started trying to walk back through all the callers of
dm_get_device, but that rabbit hole got really deep really quickly so I
didn't feel confident that changing it wouldn't break anyone.

quotactl gave me pause, as it seems to have done for you too. I was
surprised that inode permissions aren't checked, but
check_quotactl_permission does get called before actually doing
anything. I fear that adding a check of inode permissions could end up
breaking someone.

> Ugh.  Your code needs to be using __inode_permission and not
> inode_permission.
> 
> The readability or writability of a device node has little or nothing to
> do with the readability/writability of the superblock.   So I don't see
> any reason we should be checking that.

Makes sense, fixed.

Thanks,
Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Sept. 25, 2015, 5:16 p.m. UTC | #3
Argh.  This looks like morning person meets night owl.

Seth Forshee <seth.forshee@canonical.com> writes:

> On Thu, Sep 24, 2015 at 04:53:11PM -0500, Eric W. Biederman wrote:
>> Seth Forshee <seth.forshee@canonical.com> writes:
>> 
>> > When mounting a filesystem on a block device there is currently
>> > no verification that the user has appropriate access to the
>> > device file passed to mount. This has not been an issue so far
>> > since the user in question has always been root, but this must
>> > be changed before allowing unprivileged users to mount in user
>> > namespaces.
>> >
>> > To do this, a new version of lookup_bdev() is added named
>> > lookup_bdev_perm(). Both of these functions become wrappers
>> > around a common inner fucntion. The behavior of lookup_bdev() is
>> > unchanged, but calling lookup_bdev_perm() will fail if the user
>> > does not have the specified access rights to the supplied path.
>> > The permission check is skipped if the user has CAP_SYS_ADMIN to
>> > avoid any possible regressions in behavior.
>> >
>> > blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
>> > is used by mount_bdev() and mount_mtd(), so this will cause
>> > mounts on block devices to fail when the user lacks the required
>> > permissions. Other calls to blkdev_get_by_path() will all happen
>> > with root privileges, so these calls will be unaffected.
>> 
>> Good but buggy patch.
>> 
>> In the mtd bits the flags are super flags, not file mode bits,
>> which makes testing them against FMODE_READ and FMODE_WRITE is
>> incorrect.
>
> Bah, yes. Fixed.
>
>> Further it looks like quite a few more possibly all of the lookup_bdev
>> instances could use inode level permission checking.
>> 
>> Certainly code such as quotactl makes me wonder.
>
> I opted to stick to places related to mounting, but let's take a look at
> the other callers.
>
> bcache calls it in the context of sysfs writes, and those attributes are
> writable only by root. In that case the inode permission check will be
> skipped anyway, so it makes no difference either way.
>
> Device mapper calls it in dm_get_device, which is called from a bunch of
> places. I had started trying to walk back through all the callers of
> dm_get_device, but that rabbit hole got really deep really quickly so I
> didn't feel confident that changing it wouldn't break anyone.
>
> quotactl gave me pause, as it seems to have done for you too. I was
> surprised that inode permissions aren't checked, but
> check_quotactl_permission does get called before actually doing
> anything. I fear that adding a check of inode permissions could end up
> breaking someone.

My gut feel on all of this is that we should act like may_open and have
have a flag of 0 for access mode mean don't check permissions.

That way we can change all of the callers of lookup_bdev to pass an
additional argument and make it explicit what is going on but we don't
actually have to change the callers to actually perform an additional
check.

Leaving stones unturned is a good way to introduce a security hole by
accident so I don't want to leave dm_get_device unreviewed, but any
changes can be in later patches.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Sept. 25, 2015, 5:39 p.m. UTC | #4
On Fri, Sep 25, 2015 at 12:16:59PM -0500, Eric W. Biederman wrote:
> 
> Argh.  This looks like morning person meets night owl.

Indded :-)

> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > On Thu, Sep 24, 2015 at 04:53:11PM -0500, Eric W. Biederman wrote:
> >> Seth Forshee <seth.forshee@canonical.com> writes:
> >> 
> >> > When mounting a filesystem on a block device there is currently
> >> > no verification that the user has appropriate access to the
> >> > device file passed to mount. This has not been an issue so far
> >> > since the user in question has always been root, but this must
> >> > be changed before allowing unprivileged users to mount in user
> >> > namespaces.
> >> >
> >> > To do this, a new version of lookup_bdev() is added named
> >> > lookup_bdev_perm(). Both of these functions become wrappers
> >> > around a common inner fucntion. The behavior of lookup_bdev() is
> >> > unchanged, but calling lookup_bdev_perm() will fail if the user
> >> > does not have the specified access rights to the supplied path.
> >> > The permission check is skipped if the user has CAP_SYS_ADMIN to
> >> > avoid any possible regressions in behavior.
> >> >
> >> > blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
> >> > is used by mount_bdev() and mount_mtd(), so this will cause
> >> > mounts on block devices to fail when the user lacks the required
> >> > permissions. Other calls to blkdev_get_by_path() will all happen
> >> > with root privileges, so these calls will be unaffected.
> >> 
> >> Good but buggy patch.
> >> 
> >> In the mtd bits the flags are super flags, not file mode bits,
> >> which makes testing them against FMODE_READ and FMODE_WRITE is
> >> incorrect.
> >
> > Bah, yes. Fixed.
> >
> >> Further it looks like quite a few more possibly all of the lookup_bdev
> >> instances could use inode level permission checking.
> >> 
> >> Certainly code such as quotactl makes me wonder.
> >
> > I opted to stick to places related to mounting, but let's take a look at
> > the other callers.
> >
> > bcache calls it in the context of sysfs writes, and those attributes are
> > writable only by root. In that case the inode permission check will be
> > skipped anyway, so it makes no difference either way.
> >
> > Device mapper calls it in dm_get_device, which is called from a bunch of
> > places. I had started trying to walk back through all the callers of
> > dm_get_device, but that rabbit hole got really deep really quickly so I
> > didn't feel confident that changing it wouldn't break anyone.
> >
> > quotactl gave me pause, as it seems to have done for you too. I was
> > surprised that inode permissions aren't checked, but
> > check_quotactl_permission does get called before actually doing
> > anything. I fear that adding a check of inode permissions could end up
> > breaking someone.
> 
> My gut feel on all of this is that we should act like may_open and have
> have a flag of 0 for access mode mean don't check permissions.
> 
> That way we can change all of the callers of lookup_bdev to pass an
> additional argument and make it explicit what is going on but we don't
> actually have to change the callers to actually perform an additional
> check.

Sounds reasonable, I'll make that change.

> Leaving stones unturned is a good way to introduce a security hole by
> accident so I don't want to leave dm_get_device unreviewed, but any
> changes can be in later patches.

Unless I've made a mistake it shouldn't introduce security holes,
dm_get_device should behave exactly the same was as it behaves today.
Any security problems would already be present.

I can take another crack at reviewing, but it might also be good if
someone who already knows the code commented as well. As I recall I gave
up after getting several levels deep in indirect function calls where the
names of the struct members which held the function pointers were
identical at a couple of levels, so I was having a hard time knowing if
I was keeping everything straight.

Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman Sept. 25, 2015, 5:49 p.m. UTC | #5
Seth Forshee <seth.forshee@canonical.com> writes:

> On Fri, Sep 25, 2015 at 12:16:59PM -0500, Eric W. Biederman wrote:
>> 
>> Argh.  This looks like morning person meets night owl.
>
> Indded :-)
>
>> Seth Forshee <seth.forshee@canonical.com> writes:
>> 
>> > On Thu, Sep 24, 2015 at 04:53:11PM -0500, Eric W. Biederman wrote:
>> >> Seth Forshee <seth.forshee@canonical.com> writes:
>> >> 
>> >> > When mounting a filesystem on a block device there is currently
>> >> > no verification that the user has appropriate access to the
>> >> > device file passed to mount. This has not been an issue so far
>> >> > since the user in question has always been root, but this must
>> >> > be changed before allowing unprivileged users to mount in user
>> >> > namespaces.
>> >> >
>> >> > To do this, a new version of lookup_bdev() is added named
>> >> > lookup_bdev_perm(). Both of these functions become wrappers
>> >> > around a common inner fucntion. The behavior of lookup_bdev() is
>> >> > unchanged, but calling lookup_bdev_perm() will fail if the user
>> >> > does not have the specified access rights to the supplied path.
>> >> > The permission check is skipped if the user has CAP_SYS_ADMIN to
>> >> > avoid any possible regressions in behavior.
>> >> >
>> >> > blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
>> >> > is used by mount_bdev() and mount_mtd(), so this will cause
>> >> > mounts on block devices to fail when the user lacks the required
>> >> > permissions. Other calls to blkdev_get_by_path() will all happen
>> >> > with root privileges, so these calls will be unaffected.
>> >> 
>> >> Good but buggy patch.
>> >> 
>> >> In the mtd bits the flags are super flags, not file mode bits,
>> >> which makes testing them against FMODE_READ and FMODE_WRITE is
>> >> incorrect.
>> >
>> > Bah, yes. Fixed.
>> >
>> >> Further it looks like quite a few more possibly all of the lookup_bdev
>> >> instances could use inode level permission checking.
>> >> 
>> >> Certainly code such as quotactl makes me wonder.
>> >
>> > I opted to stick to places related to mounting, but let's take a look at
>> > the other callers.
>> >
>> > bcache calls it in the context of sysfs writes, and those attributes are
>> > writable only by root. In that case the inode permission check will be
>> > skipped anyway, so it makes no difference either way.
>> >
>> > Device mapper calls it in dm_get_device, which is called from a bunch of
>> > places. I had started trying to walk back through all the callers of
>> > dm_get_device, but that rabbit hole got really deep really quickly so I
>> > didn't feel confident that changing it wouldn't break anyone.
>> >
>> > quotactl gave me pause, as it seems to have done for you too. I was
>> > surprised that inode permissions aren't checked, but
>> > check_quotactl_permission does get called before actually doing
>> > anything. I fear that adding a check of inode permissions could end up
>> > breaking someone.
>> 
>> My gut feel on all of this is that we should act like may_open and have
>> have a flag of 0 for access mode mean don't check permissions.
>> 
>> That way we can change all of the callers of lookup_bdev to pass an
>> additional argument and make it explicit what is going on but we don't
>> actually have to change the callers to actually perform an additional
>> check.
>
> Sounds reasonable, I'll make that change.
>
>> Leaving stones unturned is a good way to introduce a security hole by
>> accident so I don't want to leave dm_get_device unreviewed, but any
>> changes can be in later patches.
>
> Unless I've made a mistake it shouldn't introduce security holes,
> dm_get_device should behave exactly the same was as it behaves today.
> Any security problems would already be present.

*Nod*

There are two parts to this.  Splitting this into changes that are small
enough to be correct.  And just passing in a 0 for acc_mode seems good
enough in that case.  The other question is what happens when we start
allowing unprivileged users to mount things, and in general what happens
when we start relaxing permission checks.

Frankly we need to understand how everything interacts if we are to
relax permissions safely if we don't want to be chasing security issues
for the next several years as things come to lite that we have
overlooked.

> I can take another crack at reviewing, but it might also be good if
> someone who already knows the code commented as well. As I recall I gave
> up after getting several levels deep in indirect function calls where the
> names of the struct members which held the function pointers were
> identical at a couple of levels, so I was having a hard time knowing if
> I was keeping everything straight.

For this patch I am not concerned but we do need to understand how all
of the pieces interact and that is just plain challenging.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 20c02a3b7417..a3970b0506b9 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -125,6 +125,7 @@  struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
 #ifdef CONFIG_BLOCK
 	struct block_device *bdev;
 	int ret, major;
+	int perm = 0;
 #endif
 	int mtdnr;
 
@@ -176,7 +177,11 @@  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);
+	if (flags & FMODE_READ)
+		perm |= MAY_READ;
+	if (flags & FMODE_WRITE)
+		perm |= MAY_WRITE;
+	bdev = lookup_bdev_perm(dev_name, perm);
 	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 46bd98482f71..7ca0f8911e97 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1401,9 +1401,14 @@  struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
 					void *holder)
 {
 	struct block_device *bdev;
+	int perm = 0;
 	int err;
 
-	bdev = lookup_bdev(path);
+	if (mode & FMODE_READ)
+		perm |= MAY_READ;
+	if (mode & FMODE_WRITE)
+		perm |= MAY_WRITE;
+	bdev = lookup_bdev_perm(path, perm);
 	if (IS_ERR(bdev))
 		return bdev;
 
@@ -1710,15 +1715,8 @@  int ioctl_by_bdev(struct block_device *bdev, unsigned cmd, unsigned long arg)
 
 EXPORT_SYMBOL(ioctl_by_bdev);
 
-/**
- * lookup_bdev  - lookup a struct block_device by name
- * @pathname:	special file representing the block device
- *
- * Get a reference to the blockdevice at @pathname in the current
- * namespace if possible and return it.  Return ERR_PTR(error)
- * otherwise.
- */
-struct block_device *lookup_bdev(const char *pathname)
+static struct block_device *__lookup_bdev(const char *pathname, int mask,
+					  bool check_perm)
 {
 	struct block_device *bdev;
 	struct inode *inode;
@@ -1733,6 +1731,11 @@  struct block_device *lookup_bdev(const char *pathname)
 		return ERR_PTR(error);
 
 	inode = d_backing_inode(path.dentry);
+	if (check_perm && !capable(CAP_SYS_ADMIN)) {
+		error = inode_permission(inode, mask);
+		if (error)
+			goto fail;
+	}
 	error = -ENOTBLK;
 	if (!S_ISBLK(inode->i_mode))
 		goto fail;
@@ -1750,6 +1753,35 @@  fail:
 	bdev = ERR_PTR(error);
 	goto out;
 }
+
+/*
+ * lookup_bdev_perm - lookup a struct block_device by name and check for
+ *		      access rights
+ * @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, check for the specified rights to the device
+ * at @pathname, and return it.  Return ERR_PTR(error) otherwise.
+ */
+struct block_device *lookup_bdev_perm(const char *pathname, int mask)
+{
+	return __lookup_bdev(pathname, mask, true);
+}
+EXPORT_SYMBOL(lookup_bdev_perm);
+
+/**
+ * lookup_bdev  - lookup a struct block_device by name
+ * @pathname:	special file representing the block device
+ *
+ * Get a reference to the blockdevice at @pathname in the current
+ * namespace if possible and return it.  Return ERR_PTR(error)
+ * otherwise.
+ */
+struct block_device *lookup_bdev(const char *pathname)
+{
+	return __lookup_bdev(pathname, 0, false);
+}
 EXPORT_SYMBOL(lookup_bdev);
 
 int __invalidate_device(struct block_device *bdev, bool kill_dirty)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5ec201e8308c..d03c2061bdd2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2383,6 +2383,7 @@  static inline void unregister_chrdev(unsigned int major, const char *name)
 #define BLKDEV_MAJOR_HASH_SIZE	255
 extern const char *__bdevname(dev_t, char *buffer);
 extern const char *bdevname(struct block_device *bdev, char *buffer);
+extern struct block_device *lookup_bdev_perm(const char *, int);
 extern struct block_device *lookup_bdev(const char *);
 extern void blkdev_show(struct seq_file *,off_t);