Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Thu 01-10-15 10:55:50, Eric W. Biederman wrote: > The goal if possible is to run things like docker without needed to be > root or even more fun to run docker in a container, and in general > enable nested containers. Frankly at the filesystem side we are rather far from being able to safely mount untrusted device and I don't think we'll ever be robust enough to tolerate e.g. user changing the disk while fs is using it. So would this be FUSE-only thing or is someone still hoping that general purpose filesystems will be robust enough in future? Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: auditing kdbus service names
On Thursday, August 13, 2015 04:40:52 PM Steve Grubb wrote: > On Wednesday, August 12, 2015 10:48:10 PM Paul Moore wrote: > > On Wednesday, August 12, 2015 05:38:14 PM Steve Grubb wrote: > > > On Wednesday, August 12, 2015 08:40:34 AM Paul Moore wrote: > > > > Hello all, > > > > > > > > I'm currently working on a set of LSM hooks for the new kdbus IPC > > > > mechanism and one of the things that I believe we will need to add is > > > > a new audit field for the kdbus service name (very similar to the old > > > > fashioned dbus service name). I was thinking "kdbus_svc" for the > > > > field name, any objections? > > > > > > What was used on the old dbus events? > > > > The very generic "service" field name, see the "acquire_svc" example in > > the URL below. I believe there is some value in picking a new field name > > since 1) the field name is too generic in my opinion and 2) kdbus != dbus. > > In my book, they are the same. They are programs providing services on the > bus. One thing I noticed in the dbus events is that there are a number of > user controlled fields that are not escaped. Following up on this ... Decided to just reuse "service" since the rest of the audit record will make it obvious (new obj class/perms) that the record if for a kdbus event and not a dbus event. The next patchset will include the audit bits, I'll CC the patchset here. -- paul moore security @ redhat -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/5] firmware: add an extensible system data helpers
From: "Luis R. Rodriguez" The firmware API has evolved over the years slowly, as it grows we extend it by adding new routines or at times we extend existing routines with more or less arguments. This doesn't scale well, when new arguments are added to existing routines it means we need to traverse the kernel with a slew of collateral evolutions to adjust old driver users. The firmware API is also now being used for things outside of the scope of what typically would be considered "firmware", an example here is the p54 driver enables users to provide a custom EEPROM through this interface. Another example is optional CPU microcode updates. There are other subsystems which would like to make use of the APIs for similar things but have different requirements and criteria which they'd like to be met for the requested file. If different requirements are needed it would again mean adding more arguments and making a slew of collateral evolutions, or adding yet-another-new-API-call. Instead of extending the existing firmware API even more this provides an new extensible API which can be used to supercede the old firmware API without causing a series of collateral evolutions as requirements grow. This leaves the old firmware API as-is, ignores all consideration for usermode-helpers, labels the new API to reflect its broad use outside of the scope of firmware: system data helpers, and builds on top of the original firmware core code. The new extensible "system data" set of helpers accepts that there really are only two types of requests for accessing system data: a) synchronous requests b) asynchronous requests Both of these requests may have a different set of requirements which must be met. These requirements can simply be passed as a descriptors to each type of request. The descriptor can be extended over time to support different requirements as the kernel evolves. Using the new system data helpers is only necessary if you have requirements outside of what the existing old firmware API accepts. We encourage developers to leave the old API as-is and extend the new descriptors and system data code to provide support for new features. A few simple features added as part of the new set of system data request APIs, other than making the new API easily extensible for the future: - By default the kernel will free the system data file for you after your callbacks are called, you however are allowed to request to that you wish to keep the system data file on the descriptor. This is dealt with by requiring a consumer callback for the system data file. - Allow both asynchronous and synchronous request to specify that system data files are optional. With the old APIs we had added one full API call, request_firmware_direct() just for this purpose -- although it should be noted another of its goal was to also skip the usermode helper. The system data request APIs allow for you to annotate that a system data file is optional for both synchronous or asynchronous requests through the same two basic set of APIs. - Usermode helpers are completely ignored, always - The system data request APIs currently match the old synchronous firmware API calls to refcounted firmware_class module, but it should be easy to add support now to enable also refcounting the caller's module should it be be needed. Likewise the system data request APIs match the old asynchronous firmware API call and refcounts the caller's module. In order to try to help phase out user mode helpers this makes no use of the old user mode helper code *at all*, and if we wish to can easily phase this code out with time then. Signed-off-by: Luis R. Rodriguez --- Documentation/firmware_class/system_data.txt | 71 +++ drivers/base/firmware_class.c| 291 +++ include/linux/sysdata.h | 208 +++ 3 files changed, 570 insertions(+) create mode 100644 Documentation/firmware_class/system_data.txt create mode 100644 include/linux/sysdata.h diff --git a/Documentation/firmware_class/system_data.txt b/Documentation/firmware_class/system_data.txt new file mode 100644 index ..1fe0ecd29324 --- /dev/null +++ b/Documentation/firmware_class/system_data.txt @@ -0,0 +1,71 @@ +System data requests API + + +As the kernel evolves we keep extending the firmware_class set of APIs +with more or less arguments, this creates a slew of collateral evolutions. +The set of users of firmware request APIs has also grown now to include +users which are not looking for "firmware" per se, but instead general +system data files which for one reason or another has been decided to be +kept oustide of the kernel, and/or to allow dynamic updates. The system data +request set of APIs addresses rebranding of firmware as generic system data +files, and provides a way to enable these APIs to easily be extended without +much collateral evolutions
[PATCH v2 4/5] firmware: generalize reading file contents as a helper
From: David Howells
We'll want to reuse this same code later in order to
read two separate types of file contents. This generalizes
fw_read_file() for reading a file rebrands it as fw_read_file().
This caller lets us pegs arbitrary data onto the target
buffer and size if the file is found.
While at it this cleans up the exit paths on fw_read_file().
Signed-off-by: David Howells
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_class.c | 62 +++
1 file changed, 39 insertions(+), 23 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index e10a5349aa61..cd51a90ed012 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -291,34 +291,51 @@ static const char * const fw_path[] = {
module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
MODULE_PARM_DESC(path, "customized firmware image search path with a higher
priority than default path");
-static int fw_read_file_contents(struct file *file, struct firmware_buf
*fw_buf)
+/*
+ * Read the contents of a file.
+ */
+static int fw_read_file(const char *path, void **_buf, size_t *_size)
{
- int size;
+ struct file *file;
+ size_t size;
char *buf;
int rc;
+ file = filp_open(path, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ rc = -EINVAL;
if (!S_ISREG(file_inode(file)->i_mode))
- return -EINVAL;
+ goto err_file;
size = i_size_read(file_inode(file));
if (size <= 0)
- return -EINVAL;
+ goto err_file;
+ rc = -ENOMEM;
buf = vmalloc(size);
if (!buf)
- return -ENOMEM;
+ goto err_file;
+
rc = kernel_read(file, 0, buf, size);
+ if (rc < 0)
+ goto err_buf;
if (rc != size) {
- if (rc > 0)
- rc = -EIO;
- goto fail;
+ rc = -EIO;
+ goto err_buf;
}
+
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
- goto fail;
- fw_buf->data = buf;
- fw_buf->size = size;
+ goto err_buf;
+
+ *_buf = buf;
+ *_size = size;
return 0;
-fail:
+
+err_buf:
vfree(buf);
+err_file:
+ fput(file);
return rc;
}
@@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
}
static int fw_get_filesystem_firmware(struct device *device,
- struct firmware_buf *buf)
+ struct firmware_buf *buf)
{
int i, len;
int rc = -ENOENT;
- char *path;
+ char *path = NULL;
path = __getname();
if (!path)
return -ENOMEM;
+ /*
+* Try each possible firmware blob in turn till one doesn't produce
+* ENOENT.
+*/
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
- struct file *file;
-
/* skip the unset customized path */
if (!fw_path[i][0])
continue;
@@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device
*device,
break;
}
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- continue;
- rc = fw_read_file_contents(file, buf);
- fput(file);
+ rc = fw_read_file(path, &buf->data, &buf->size);
if (rc == 0) {
dev_dbg(device, "system data: direct-loading firmware
%s\n",
buf->fw_id);
fw_finish_direct_load(device, buf);
goto out;
- } else
+ } else if (rc != -ENOENT) {
dev_warn(device, "system data, attempted to load %s,
but failed with error %d\n",
path, rc);
+ goto out;
+ }
}
out:
__putname(path);
-
return rc;
}
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] firmware: fold successful fw read early
From: David Howells
We'll be folding in some more checks on fw_read_file_contents(),
this will make the success case easier to follow.
Signed-off-by: David Howells
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_class.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d8148aa89b01..e10a5349aa61 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device
*device,
continue;
rc = fw_read_file_contents(file, buf);
fput(file);
- if (rc)
+ if (rc == 0) {
+ dev_dbg(device, "system data: direct-loading firmware
%s\n",
+ buf->fw_id);
+ fw_finish_direct_load(device, buf);
+ goto out;
+ } else
dev_warn(device, "system data, attempted to load %s,
but failed with error %d\n",
path, rc);
- else
- break;
}
+out:
__putname(path);
- if (!rc) {
- dev_dbg(device, "system data: direct-loading firmware %s\n",
- buf->fw_id);
- fw_finish_direct_load(device, buf);
- }
-
return rc;
}
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] firmware: move completing fw into a helper
From: "Luis R. Rodriguez"
This will be re-used later through a new extensible interface.
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_class.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6f5fcda71a60..d8148aa89b01 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,6 +322,15 @@ fail:
return rc;
}
+static void fw_finish_direct_load(struct device *device,
+ struct firmware_buf *buf)
+{
+ mutex_lock(&fw_lock);
+ set_bit(FW_STATUS_DONE, &buf->status);
+ complete_all(&buf->completion);
+ mutex_unlock(&fw_lock);
+}
+
static int fw_get_filesystem_firmware(struct device *device,
struct firmware_buf *buf)
{
@@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device
*device,
if (!rc) {
dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
- mutex_lock(&fw_lock);
- set_bit(FW_STATUS_DONE, &buf->status);
- complete_all(&buf->completion);
- mutex_unlock(&fw_lock);
+ fw_finish_direct_load(device, buf);
}
return rc;
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] firmware: generalize "firmware" as "system data" helpers
From: "Luis R. Rodriguez"
Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.
Annotate this before we extend firmare_class more,
as this is expected. We want to generalize the code
as much as possible.
Cc: Rusty Russell
Cc: Andrew Morton
Cc: Greg Kroah-Hartman
Cc: David Howells
Cc: Kees Cook
Cc: Casey Schaufler
Cc: Ming Lei
Cc: Takashi Iwai
Cc: Vojtěch Pavlík
Cc: Kyle McMartin
Cc: Matthew Garrett
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez
---
drivers/base/firmware_class.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450e75bd..6f5fcda71a60 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device
*device,
rc = fw_read_file_contents(file, buf);
fput(file);
if (rc)
- dev_warn(device, "firmware, attempted to load %s, but
failed with error %d\n",
- path, rc);
+ dev_warn(device, "system data, attempted to load %s,
but failed with error %d\n",
+path, rc);
else
break;
}
__putname(path);
if (!rc) {
- dev_dbg(device, "firmware: direct-loading firmware %s\n",
+ dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
mutex_lock(&fw_lock);
set_bit(FW_STATUS_DONE, &buf->status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p,
const char *name,
}
if (fw_get_builtin_firmware(firmware, name)) {
- dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+ dev_dbg(device, "system data: using built-in system data%s\n",
name);
return 0; /* assigned */
}
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/5] firmware_class: extensible firmware API
From: "Luis R. Rodriguez" This v2 series pushes together a few simple code shifts me and David worked on with a new extensible firmware API. I have bundled these changes together as in my last v1 series [0] it was not clear why the first simple set of changes were being made, and I had sent out the extensible firmware API only as an RFC and separately. I've bundled these two together, this should make it clearer now why we made the first set of changes, the last patch is the meat of the work to consider and review here. Its important to realize that this series does not include firmware signing support. The exact architecture for that is still being discussed -- if we don't all agree on the exact architecture of firmware signing through mailing list discussions one possibility is to just discuss it as one of the topics for the upcoming kernel summit. Now, regardless of the outcome of the exact architecture and design of firmware signing -- a few thing are clear though: 1) we keep extending the firmware API as we come up with more needs, when we do this it means we have a series of unnecessary collateral evolutions on drivers... 2) the user mode helper code needs to die off and although some distributions are now disabling support for it and systemd ripped out support for it, Josh Boyer recently noted that drivers other than dell-rbu now also select FW_LOADER_USER_HELPER_FALLBACK [1]... we should really just compartamentalize such use and phase that out slowly. The extensible firmware API then is being introduced both as a stepping stone to expected changes to the firmware API for Linux firmware signing support and as a way for us to phase out the user mode helper. My original approach to firmware signing was to support firmware signing behind the scenes automatically for all drivers when enabled via Kconfig, with one default key -- similar to how module signing support works. In this original design it was only a side consideration to support custom keys for firmware signing, for instance I had envisioned that we could replace CRDA with a simple firmware request call with our own custom 802.11 key requirements, which would be orthogonal to what distributions decide for "general firmware signing". Upon review though those who have expressed interest in firmware signing have stated interest to be able to specify their own driver key signing criteria from the start. If we end up requiring key requirements to *always* be specified we'll essentially be extending the number of arguments to the firmware API from the start. Instead of having *all* drivers be modified that use the old firmware API, or adding yet another new call, an extensible firmware API would enable only changes to be made for drivers that do want to explicitly opt-in for support for firmware signing, with an added bonus of also avoiding the user mode helper, and enabling this new API to be easily extensible in the future without other collateral evolutions. [0] http://lkml.kernel.org/r/1438725604-22795-1-git-send-email-mcg...@do-not-panic.com [1] http://lkml.kernel.org/r/ca+5pva6togj5ibotgk5casy9vuaqc8sxywkex+ojwc51uev...@mail.gmail.com David Howells (2): firmware: fold successful fw read early firmware: generalize reading file contents as a helper Luis R. Rodriguez (3): firmware: generalize "firmware" as "system data" helpers firmware: move completing fw into a helper firmware: add an extensible system data helpers Documentation/firmware_class/system_data.txt | 71 + drivers/base/firmware_class.c| 385 --- include/linux/sysdata.h | 208 +++ 3 files changed, 627 insertions(+), 37 deletions(-) create mode 100644 Documentation/firmware_class/system_data.txt create mode 100644 include/linux/sysdata.h -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
Mike Snitzer writes: > What layer establishes access rights to historically root-only > priviledged block devices? Is it user namespaces? Block devices are weird. Mounts historically have not checked the permissions on the block devices because a mounter has CAP_SYS_ADMIN. Unprivileged users are allowes to read/write block devices if someone has given them permissions on the device node in the filesystem. The thinking with this patchset is to start performing the normal block device access permission checks when mounting filesystems when the mounter does not have the global CAP_SYS_ADMIN permission. The truth is we are not much past the point of realizing that there were no permission checks to use the actual block device passed in to mount, so we could still be missing something. There is a lot going on with dm, md, and lvm. I don't know if the model of just look at the block device inode and perform the permission checks is good enough. > I haven't kept up with user namespaces as it relates to stacking block > drivers like DM. But I'm happy to come up to speed and at the same time > help you verify all works as expected with DM blocks devices... We are just getting there. But if you can help that would be great. The primary concern with dm is what happens when unprivileged users get ahold of the code, and what happens when evil users corrupt the on-disk format. In principle dm like loop should be safe to use if there are not bugs that make it unsafe for unprivileged users to access the code. The goal if possible is to run things like docker without needed to be root or even more fun to run docker in a container, and in general enable nested containers. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Thu, Oct 01, 2015 at 10:40:08AM -0500, Eric W. Biederman wrote: > Seth Forshee 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 fix this, add an argument to lookup_bdev() to specify the > > required permissions. If the mask of permissions is zero, or > > if the user has CAP_SYS_ADMIN, the permission check is skipped, > > otherwise the lookup fails if the user does not have the > > specified access rights for the inode at the supplied path. > > > > Callers associated with mounting are updated to pass permission > > masks to lookup_bdev() so that these mounts will fail for an > > unprivileged user who lacks permissions for the block device > > inode. All other callers pass 0 to maintain their current > > behaviors. > > > > Seth can you split this patch? > > One patch to add an argument to lookup_bdev, > and then for each kind of callsite a follow-on patch (if we are ready > for that). > > That will separate the logical changes and make things easier to track > via bisect and more importantly easier to review things. Sure, I'll do that. Seth -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
Seth Forshee 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 fix this, add an argument to lookup_bdev() to specify the
> required permissions. If the mask of permissions is zero, or
> if the user has CAP_SYS_ADMIN, the permission check is skipped,
> otherwise the lookup fails if the user does not have the
> specified access rights for the inode at the supplied path.
>
> Callers associated with mounting are updated to pass permission
> masks to lookup_bdev() so that these mounts will fail for an
> unprivileged user who lacks permissions for the block device
> inode. All other callers pass 0 to maintain their current
> behaviors.
>
Seth can you split this patch?
One patch to add an argument to lookup_bdev,
and then for each kind of callsite a follow-on patch (if we are ready
for that).
That will separate the logical changes and make things easier to track
via bisect and more importantly easier to review things.
Eric
> Signed-off-by: Seth Forshee
> ---
> drivers/md/bcache/super.c | 2 +-
> drivers/md/dm-table.c | 2 +-
> drivers/mtd/mtdsuper.c| 6 +-
> fs/block_dev.c| 18 +++---
> fs/quota/quota.c | 2 +-
> include/linux/fs.h| 2 +-
> 6 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 679a093a3bf6..e8287b0d1dac 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1926,7 +1926,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 e76ed003769e..35bb3ea4cbe2 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path,
> fmode_t mode,
> BUG_ON(!t);
>
> /* convert the path to a device */
> - bdev = lookup_bdev(path);
> + bdev = lookup_bdev(path, 0);
> if (IS_ERR(bdev)) {
> dev = name_to_dev_t(path);
> if (!dev)
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index 20c02a3b7417..5d7e7705fed8 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;
> #endif
> int mtdnr;
>
> @@ -176,7 +177,10 @@ 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);
> + perm = MAY_READ;
> + if (!(flags & MS_RDONLY))
> + perm |= MAY_WRITE;
> + bdev = lookup_bdev(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 26cee058dc02..54d94cd64577 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1394,9 +1394,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(path, perm);
> if (IS_ERR(bdev))
> return bdev;
>
> @@ -1706,12 +1711,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, in
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Thu, Oct 01, 2015 at 09:40:52AM -0400, Mike Snitzer wrote:
> On Thu, Oct 01 2015 at 8:55am -0400,
> Seth Forshee wrote:
>
> > On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > > On Wed, Sep 30 2015 at 4:15pm -0400,
> > > Seth Forshee wrote:
> > >
> > > > 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 fix this, add an argument to lookup_bdev() to specify the
> > > > required permissions. If the mask of permissions is zero, or
> > > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > > otherwise the lookup fails if the user does not have the
> > > > specified access rights for the inode at the supplied path.
> > > >
> > > > Callers associated with mounting are updated to pass permission
> > > > masks to lookup_bdev() so that these mounts will fail for an
> > > > unprivileged user who lacks permissions for the block device
> > > > inode. All other callers pass 0 to maintain their current
> > > > behaviors.
> > > >
> > > > Signed-off-by: Seth Forshee
> > > > ---
> > > > drivers/md/bcache/super.c | 2 +-
> > > > drivers/md/dm-table.c | 2 +-
> > > > drivers/mtd/mtdsuper.c| 6 +-
> > > > fs/block_dev.c| 18 +++---
> > > > fs/quota/quota.c | 2 +-
> > > > include/linux/fs.h| 2 +-
> > > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > index e76ed003769e..35bb3ea4cbe2 100644
> > > > --- a/drivers/md/dm-table.c
> > > > +++ b/drivers/md/dm-table.c
> > > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char
> > > > *path, fmode_t mode,
> > > > BUG_ON(!t);
> > > >
> > > > /* convert the path to a device */
> > > > - bdev = lookup_bdev(path);
> > > > + bdev = lookup_bdev(path, 0);
> > > > if (IS_ERR(bdev)) {
> > > > dev = name_to_dev_t(path);
> > > > if (!dev)
> > >
> > > Given dm_get_device() is passed @mode why not have it do something like
> > > you did in blkdev_get_by_path()? e.g.:
> >
> > I only dealt with code related to mounting in this patch since that's
> > what I'm working on. I have it on my TODO list to consider converting
> > other callers of lookup_bdev. But if you're sure doing so makes sense
> > for dm_get_device and that it won't cause regressions then I could add a
> > patch for it.
>
> OK, dm_get_device() is called in DM device activation path (by tools
> like lvm2).
>
> After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
> call chain:
> dm_get_device -> dm_get_table_device -> open_table_device ->
> blkdev_get_by_dev
>
> Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
> to do this checking also.
>
> However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
> new virtual block devices are created that layer ontop of the
> traditional block devices. This level of indirection may cause your
> lookup_bdev() check to go on to succeed (if access constraints were not
> established on the upper level dm or md device?). I'm just thinking
> outloud here: but have you verified your changes work as intended on
> devices created with either lvm2 or mdadm?
>
> What layer establishes access rights to historically root-only
> priviledged block devices? Is it user namespaces?
I'm going to start with this last question and work my way backwards.
Who determines access rights to the block devices varies to some degree.
Any normal user could unshare the user/mount namespaces and still see
all the block devices in /dev. If we're going to allow that user to then
mount block devices in their private namespaces, the kernel must verify
that the user has appropriate permissions for the block device inode.
That's the point of this patch. In a container context the host process
which sets up the container might share some block devices into the
container via bind mounts, in which case it would be responsible for
setting up access rights (both via inode permissions and potentially via
devcgroups). I also have some patches I'm working on for a loop psuedo
filesystem which would allow a container to allocate loop devices for
its own use (via the loop-control device), in which case the kernel
creates a device node in the loopfs filesystem from which the new loop
device was allocated, owned by the root user in the user namespace
associated with the loopfs superblock.
Obviously a DM block device is more complicated than a traditional block
device. This patch should ensure that if an unprivileged user tries to
mount a DM blkdev that it fails if the user la
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Thu, Oct 01 2015 at 8:55am -0400,
Seth Forshee wrote:
> On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > On Wed, Sep 30 2015 at 4:15pm -0400,
> > Seth Forshee wrote:
> >
> > > 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 fix this, add an argument to lookup_bdev() to specify the
> > > required permissions. If the mask of permissions is zero, or
> > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > otherwise the lookup fails if the user does not have the
> > > specified access rights for the inode at the supplied path.
> > >
> > > Callers associated with mounting are updated to pass permission
> > > masks to lookup_bdev() so that these mounts will fail for an
> > > unprivileged user who lacks permissions for the block device
> > > inode. All other callers pass 0 to maintain their current
> > > behaviors.
> > >
> > > Signed-off-by: Seth Forshee
> > > ---
> > > drivers/md/bcache/super.c | 2 +-
> > > drivers/md/dm-table.c | 2 +-
> > > drivers/mtd/mtdsuper.c| 6 +-
> > > fs/block_dev.c| 18 +++---
> > > fs/quota/quota.c | 2 +-
> > > include/linux/fs.h| 2 +-
> > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index e76ed003769e..35bb3ea4cbe2 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char
> > > *path, fmode_t mode,
> > > BUG_ON(!t);
> > >
> > > /* convert the path to a device */
> > > - bdev = lookup_bdev(path);
> > > + bdev = lookup_bdev(path, 0);
> > > if (IS_ERR(bdev)) {
> > > dev = name_to_dev_t(path);
> > > if (!dev)
> >
> > Given dm_get_device() is passed @mode why not have it do something like
> > you did in blkdev_get_by_path()? e.g.:
>
> I only dealt with code related to mounting in this patch since that's
> what I'm working on. I have it on my TODO list to consider converting
> other callers of lookup_bdev. But if you're sure doing so makes sense
> for dm_get_device and that it won't cause regressions then I could add a
> patch for it.
OK, dm_get_device() is called in DM device activation path (by tools
like lvm2).
After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
call chain:
dm_get_device -> dm_get_table_device -> open_table_device -> blkdev_get_by_dev
Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
to do this checking also.
However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
new virtual block devices are created that layer ontop of the
traditional block devices. This level of indirection may cause your
lookup_bdev() check to go on to succeed (if access constraints were not
established on the upper level dm or md device?). I'm just thinking
outloud here: but have you verified your changes work as intended on
devices created with either lvm2 or mdadm?
What layer establishes access rights to historically root-only
priviledged block devices? Is it user namespaces?
I haven't kept up with user namespaces as it relates to stacking block
drivers like DM. But I'm happy to come up to speed and at the same time
help you verify all works as expected with DM blocks devices...
Mike
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> On Wed, Sep 30 2015 at 4:15pm -0400,
> Seth Forshee wrote:
>
> > 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 fix this, add an argument to lookup_bdev() to specify the
> > required permissions. If the mask of permissions is zero, or
> > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > otherwise the lookup fails if the user does not have the
> > specified access rights for the inode at the supplied path.
> >
> > Callers associated with mounting are updated to pass permission
> > masks to lookup_bdev() so that these mounts will fail for an
> > unprivileged user who lacks permissions for the block device
> > inode. All other callers pass 0 to maintain their current
> > behaviors.
> >
> > Signed-off-by: Seth Forshee
> > ---
> > drivers/md/bcache/super.c | 2 +-
> > drivers/md/dm-table.c | 2 +-
> > drivers/mtd/mtdsuper.c| 6 +-
> > fs/block_dev.c| 18 +++---
> > fs/quota/quota.c | 2 +-
> > include/linux/fs.h| 2 +-
> > 6 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index e76ed003769e..35bb3ea4cbe2 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char
> > *path, fmode_t mode,
> > BUG_ON(!t);
> >
> > /* convert the path to a device */
> > - bdev = lookup_bdev(path);
> > + bdev = lookup_bdev(path, 0);
> > if (IS_ERR(bdev)) {
> > dev = name_to_dev_t(path);
> > if (!dev)
>
> Given dm_get_device() is passed @mode why not have it do something like
> you did in blkdev_get_by_path()? e.g.:
I only dealt with code related to mounting in this patch since that's
what I'm working on. I have it on my TODO list to consider converting
other callers of lookup_bdev. But if you're sure doing so makes sense
for dm_get_device and that it won't cause regressions then I could add a
patch for it.
Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
