| Message ID | 20211201163708.3578176-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] device mapper: Add builtin function dm_get_status() | expand |
> From: Roberto Sassu > Sent: Wednesday, December 1, 2021 5:37 PM > Users of the device mapper driver might want to obtain a device status, > with status types defined in the status_type_t enumerator. > > If a function to get the status is exported by the device mapper, when > compiled as a module, it is not suitable to use by callers compiled builtin > in the kernel. > > Introduce the real function to get the status, _dm_get_status(), in the > device mapper module, and add the stub dm_get_status() in dm-builtin.c, so > that it can be invoked by builtin callers. > > The stub calls the real function if the device mapper is compiled builtin > or the module has been loaded. Calls to the real function are safely > disabled if the module is unloaded. The race condition is avoided by > incrementing the reference count of the module. > > _dm_get_status() invokes the status() method for each device mapper table, > which writes a string to the supplied buffer as output. The buffer might > contain multiple strings concatenated together. If there is not enough > space available, the string is truncated and a termination character is put > at the end. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > drivers/md/dm-builtin.c | 13 +++++++ > drivers/md/dm-core.h | 5 +++ > drivers/md/dm.c | 71 +++++++++++++++++++++++++++++++++++ > include/linux/device-mapper.h | 3 ++ > 4 files changed, 92 insertions(+) > > diff --git a/drivers/md/dm-builtin.c b/drivers/md/dm-builtin.c > index 8eb52e425141..cc1e9c27ab41 100644 > --- a/drivers/md/dm-builtin.c > +++ b/drivers/md/dm-builtin.c > @@ -47,3 +47,16 @@ void dm_kobject_release(struct kobject *kobj) > } > > EXPORT_SYMBOL(dm_kobject_release); > + > +dm_get_status_fn status_fn; > +EXPORT_SYMBOL(status_fn); > + > +ssize_t dm_get_status(dev_t dev, status_type_t type, const char > *target_name, > + u8 *buf, size_t buf_len) > +{ > + if (status_fn) > + return status_fn(dev, type, target_name, buf, buf_len); > + > + return -EOPNOTSUPP; > +} > +EXPORT_SYMBOL(dm_get_status); > diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h > index b855fef4f38a..6600ec260558 100644 > --- a/drivers/md/dm-core.h > +++ b/drivers/md/dm-core.h > @@ -259,4 +259,9 @@ extern atomic_t dm_global_event_nr; > extern wait_queue_head_t dm_global_eventq; > void dm_issue_global_event(void); > > +typedef ssize_t (*dm_get_status_fn)(dev_t dev, status_type_t type, > + const char *target_name, u8 *buf, > + size_t buf_len); > + > +extern dm_get_status_fn status_fn; > #endif > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 662742a310cb..55e59a4e3661 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -192,6 +192,74 @@ static unsigned dm_get_numa_node(void) > DM_NUMA_NODE, > num_online_nodes() - 1); > } > > +static ssize_t _dm_get_status(dev_t dev, status_type_t type, > + const char *target_name, u8 *buf, size_t buf_len) > +{ > + struct mapped_device *md; > + struct dm_table *table; > + u8 *buf_ptr = buf; > + ssize_t len, res = 0; > + int srcu_idx, num_targets, i; > + > + if (buf_len > INT_MAX) > + return -EINVAL; > + > + if (!buf_len) > + return buf_len; > + > + if (!try_module_get(THIS_MODULE)) > + return -EBUSY; > + > + md = dm_get_md(dev); > + if (!md) { > + res = -ENOENT; > + goto out_module; > + } > + > + table = dm_get_live_table(md, &srcu_idx); > + if (!table) { > + res = -ENOENT; > + goto out_md; > + } > + > + memset(buf, 0, buf_len); > + > + num_targets = dm_table_get_num_targets(table); > + > + for (i = 0; i < num_targets; i++) { > + struct dm_target *ti = dm_table_get_target(table, i); > + > + if (!ti) > + continue; > + > + if (target_name && strcmp(ti->type->name, target_name)) > + continue; > + > + if (!ti->type->status) > + continue; > + > + ti->type->status(ti, type, 0, buf_ptr, buf + buf_len - buf_ptr); > + > + if (!*buf_ptr) > + continue; > + > + len = strlen(buf_ptr); > + buf_ptr += len + 1; > + > + if (buf_ptr == buf + buf_len) > + break; > + > + res += len + 1; The line above before the check. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua > + } > + > + dm_put_live_table(md, srcu_idx); > +out_md: > + dm_put(md); > +out_module: > + module_put(THIS_MODULE); > + return res; > +} > + > static int __init local_init(void) > { > int r; > @@ -275,6 +343,7 @@ static int __init dm_init(void) > goto bad; > } > > + status_fn = _dm_get_status; > return 0; > bad: > while (i--) > @@ -287,6 +356,8 @@ static void __exit dm_exit(void) > { > int i = ARRAY_SIZE(_exits); > > + status_fn = NULL; > + > while (i--) > _exits[i](); > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > index a7df155ea49b..d97b296d3104 100644 > --- a/include/linux/device-mapper.h > +++ b/include/linux/device-mapper.h > @@ -487,6 +487,9 @@ int dm_report_zones(struct block_device *bdev, > sector_t start, sector_t sector, > struct dm_report_zones_args *args, unsigned int nr_zones); > #endif /* CONFIG_BLK_DEV_ZONED */ > > +ssize_t dm_get_status(dev_t dev, status_type_t type, const char > *target_name, > + u8 *buf, size_t buf_len); > + > /* > * Device mapper functions to parse and create devices specified by the > * parameter "dm-mod.create=" > -- > 2.32.0
On Wed, Dec 01, 2021 at 05:37:08PM +0100, Roberto Sassu wrote: > Users of the device mapper driver might want to obtain a device status, > with status types defined in the status_type_t enumerator. The patch looks really odd. And without the corresponding user of your new functionality it is entirely unreviewable anyway.
> From: Christoph Hellwig [mailto:hch@infradead.org] > Sent: Thursday, December 2, 2021 8:21 AM > On Wed, Dec 01, 2021 at 05:37:08PM +0100, Roberto Sassu wrote: > > Users of the device mapper driver might want to obtain a device status, > > with status types defined in the status_type_t enumerator. > > The patch looks really odd. And without the corresponding user of your > new functionality it is entirely unreviewable anyway. Hi Christoph ok, I will send it together with a patch for a not yet accepted software, Integrity Policy Enforcement (IPE), that will be the primary user of the introduced functionality. Regarding the patch itself, could you please provide a more detailed explanation? Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
On Thu, Dec 02, 2021 at 07:59:38AM +0000, Roberto Sassu wrote: > ok, I will send it together with a patch for a not yet accepted > software, Integrity Policy Enforcement (IPE), that will be > the primary user of the introduced functionality. > > Regarding the patch itself, could you please provide a more > detailed explanation? We don't build things into the kernel just as hooks. So in doubt you need to restructured the code. And that a security module pokes into a random block driver is a big hint that whatever you're trying to do is completely broken.
> From: Christoph Hellwig [mailto:hch@infradead.org] > Sent: Thursday, December 2, 2021 9:44 AM > On Thu, Dec 02, 2021 at 07:59:38AM +0000, Roberto Sassu wrote: > > ok, I will send it together with a patch for a not yet accepted > > software, Integrity Policy Enforcement (IPE), that will be > > the primary user of the introduced functionality. > > > > Regarding the patch itself, could you please provide a more > > detailed explanation? > > We don't build things into the kernel just as hooks. So in doubt you > need to restructured the code. And that a security module pokes into > a random block driver is a big hint that whatever you're trying to do is > completely broken. I will add more context to the discussion. The problem being solved is how to grant access to files which satisfy a property defined in the policy. For example, a policy enforced by IPE could be: policy_name="AllowDMVerityKmodules" policy_version=0.0.1 DEFAULT action=ALLOW DEFAULT op=KMODULE action=DENY op=KMODULE dmverity_roothash=3c64aae64ae5e8ca781df4d1fbff7c02cb78c4f18a79198263db192cc7f7ba11 action=ALLOW This would require IPE to obtain somehow this property. So far, there are two different approaches. The first one, proposed by the IPE authors was to define a new LSM hook for block devices, to append a security blob for those devices, and to store the dm-verity root digest as soon as this information can be determined. IPE will then access the security blob at run-time and will match the blob content with the property value in the policy. The second one I'm proposing is to directly retrieve the information at run-time, when files are accessed, and to possibly cache the result of the evaluation per filesystem. This would avoid to the introduction of a new LSM hook and to append a security blob for the purpose of passing information from the device mapper driver to IPE. Security blobs are usually used to store LSM-specific information such as a label (or a reference of it). Sometimes, when the label must be stored persistently, the subsystem responsible for this task, such as the VFS, uses subsystem-defined methods to retrieve the label from the storage and copy it to the security blob. In this case, it is not an LSM-specific information but rather an existing property of another subsystem IPE is interested in. Since LSMs need anyway to inspect the object before making the security decision, they could directly retrieve the information that is already available, instead of making it redundant. Even if I would prefer the second option, it would be fine for me if the first is adopted. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
On Thu, Dec 02, 2021 at 09:29:52AM +0000, Roberto Sassu wrote: > The problem being solved is how to grant access to files > which satisfy a property defined in the policy. If you have want to enforce access to files in the block layer using a specific stacking block driver you don't just have one layering violation but a bunch of them. Please go back to the drawing board.
> From: Christoph Hellwig [mailto:hch@infradead.org] > Sent: Friday, December 3, 2021 7:52 AM > On Thu, Dec 02, 2021 at 09:29:52AM +0000, Roberto Sassu wrote: > > The problem being solved is how to grant access to files > > which satisfy a property defined in the policy. > > If you have want to enforce access to files in the block layer using > a specific stacking block driver you don't just have one layering > violation but a bunch of them. Please go back to the drawing board. Ok. I write my thoughts here, so that it is easier to align. dm-verity provides block-level integrity, which means that the block layer itself is responsible to not pass data to the upper layer, the filesystem, if a block is found corrupted. The dm-verity root digest represents the immutable state of the block device. dm-verity is still responsible to enforce accesses to the block device according to the root digest passed at device setup time. Nothing changes, the block layer still detects data corruption against the passed reference value. The task of the security layer is to decide whether or not the root digest passed at device setup time is acceptable, e.g. it represents a device containing genuine files coming from a software vendor. The mandatory policy can be enforced at different layers, depending on whether the security controls are placed. A possibility would be to deny mounting block devices that don't satisfy the mandatory policy. However, if the mandatory policy wants only to restrict execution of approved files and allowing the rest, making the decision at the block layer is too coarse and restrictive. It would force the user to mount only approved block devices. The security layer must operate on files to enforce this policy. Now probably there is the part where there is no agreement. The integrity property of a block device applies also to the files on the filesystem mounted from that device. User space programs cannot access files in that filesystem coming from a device with a different dm-verity root digest, or files stored in a corrupted block device. If what I wrote is correct, that the integrity property is preserved across the layers, this would give enough flexibility to enforce policies at a higher layer, although that property is guaranteed by a lower layer. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
> From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Friday, December 3, 2021 11:20 AM > > From: Christoph Hellwig [mailto:hch@infradead.org] > > Sent: Friday, December 3, 2021 7:52 AM > > On Thu, Dec 02, 2021 at 09:29:52AM +0000, Roberto Sassu wrote: > > > The problem being solved is how to grant access to files > > > which satisfy a property defined in the policy. > > > > If you have want to enforce access to files in the block layer using > > a specific stacking block driver you don't just have one layering > > violation but a bunch of them. Please go back to the drawing board. > > Ok. I write my thoughts here, so that it is easier to align. > > dm-verity provides block-level integrity, which means that > the block layer itself is responsible to not pass data to the > upper layer, the filesystem, if a block is found corrupted. > > The dm-verity root digest represents the immutable state > of the block device. dm-verity is still responsible to enforce > accesses to the block device according to the root digest > passed at device setup time. Nothing changes, the block > layer still detects data corruption against the passed > reference value. > > The task of the security layer is to decide whether or not > the root digest passed at device setup time is acceptable, > e.g. it represents a device containing genuine files coming > from a software vendor. > > The mandatory policy can be enforced at different layers, > depending on whether the security controls are placed. > A possibility would be to deny mounting block devices that > don't satisfy the mandatory policy. > > However, if the mandatory policy wants only to restrict > execution of approved files and allowing the rest, making > the decision at the block layer is too coarse and restrictive. > It would force the user to mount only approved block > devices. The security layer must operate on files to enforce > this policy. > > Now probably there is the part where there is no agreement. > > The integrity property of a block device applies also to the > files on the filesystem mounted from that device. User space > programs cannot access files in that filesystem coming from a > device with a different dm-verity root digest, or files stored > in a corrupted block device. > > If what I wrote is correct, that the integrity property is preserved > across the layers, this would give enough flexibility to enforce > policies at a higher layer, although that property is guaranteed > by a lower layer. Hi Christoph did I address your concerns? If yes, I could send the new patch set, including the patch that uses the new functionality. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
diff --git a/drivers/md/dm-builtin.c b/drivers/md/dm-builtin.c index 8eb52e425141..cc1e9c27ab41 100644 --- a/drivers/md/dm-builtin.c +++ b/drivers/md/dm-builtin.c @@ -47,3 +47,16 @@ void dm_kobject_release(struct kobject *kobj) } EXPORT_SYMBOL(dm_kobject_release); + +dm_get_status_fn status_fn; +EXPORT_SYMBOL(status_fn); + +ssize_t dm_get_status(dev_t dev, status_type_t type, const char *target_name, + u8 *buf, size_t buf_len) +{ + if (status_fn) + return status_fn(dev, type, target_name, buf, buf_len); + + return -EOPNOTSUPP; +} +EXPORT_SYMBOL(dm_get_status); diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h index b855fef4f38a..6600ec260558 100644 --- a/drivers/md/dm-core.h +++ b/drivers/md/dm-core.h @@ -259,4 +259,9 @@ extern atomic_t dm_global_event_nr; extern wait_queue_head_t dm_global_eventq; void dm_issue_global_event(void); +typedef ssize_t (*dm_get_status_fn)(dev_t dev, status_type_t type, + const char *target_name, u8 *buf, + size_t buf_len); + +extern dm_get_status_fn status_fn; #endif diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 662742a310cb..55e59a4e3661 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -192,6 +192,74 @@ static unsigned dm_get_numa_node(void) DM_NUMA_NODE, num_online_nodes() - 1); } +static ssize_t _dm_get_status(dev_t dev, status_type_t type, + const char *target_name, u8 *buf, size_t buf_len) +{ + struct mapped_device *md; + struct dm_table *table; + u8 *buf_ptr = buf; + ssize_t len, res = 0; + int srcu_idx, num_targets, i; + + if (buf_len > INT_MAX) + return -EINVAL; + + if (!buf_len) + return buf_len; + + if (!try_module_get(THIS_MODULE)) + return -EBUSY; + + md = dm_get_md(dev); + if (!md) { + res = -ENOENT; + goto out_module; + } + + table = dm_get_live_table(md, &srcu_idx); + if (!table) { + res = -ENOENT; + goto out_md; + } + + memset(buf, 0, buf_len); + + num_targets = dm_table_get_num_targets(table); + + for (i = 0; i < num_targets; i++) { + struct dm_target *ti = dm_table_get_target(table, i); + + if (!ti) + continue; + + if (target_name && strcmp(ti->type->name, target_name)) + continue; + + if (!ti->type->status) + continue; + + ti->type->status(ti, type, 0, buf_ptr, buf + buf_len - buf_ptr); + + if (!*buf_ptr) + continue; + + len = strlen(buf_ptr); + buf_ptr += len + 1; + + if (buf_ptr == buf + buf_len) + break; + + res += len + 1; + } + + dm_put_live_table(md, srcu_idx); +out_md: + dm_put(md); +out_module: + module_put(THIS_MODULE); + return res; +} + static int __init local_init(void) { int r; @@ -275,6 +343,7 @@ static int __init dm_init(void) goto bad; } + status_fn = _dm_get_status; return 0; bad: while (i--) @@ -287,6 +356,8 @@ static void __exit dm_exit(void) { int i = ARRAY_SIZE(_exits); + status_fn = NULL; + while (i--) _exits[i](); diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h index a7df155ea49b..d97b296d3104 100644 --- a/include/linux/device-mapper.h +++ b/include/linux/device-mapper.h @@ -487,6 +487,9 @@ int dm_report_zones(struct block_device *bdev, sector_t start, sector_t sector, struct dm_report_zones_args *args, unsigned int nr_zones); #endif /* CONFIG_BLK_DEV_ZONED */ +ssize_t dm_get_status(dev_t dev, status_type_t type, const char *target_name, + u8 *buf, size_t buf_len); + /* * Device mapper functions to parse and create devices specified by the * parameter "dm-mod.create="
Users of the device mapper driver might want to obtain a device status, with status types defined in the status_type_t enumerator. If a function to get the status is exported by the device mapper, when compiled as a module, it is not suitable to use by callers compiled builtin in the kernel. Introduce the real function to get the status, _dm_get_status(), in the device mapper module, and add the stub dm_get_status() in dm-builtin.c, so that it can be invoked by builtin callers. The stub calls the real function if the device mapper is compiled builtin or the module has been loaded. Calls to the real function are safely disabled if the module is unloaded. The race condition is avoided by incrementing the reference count of the module. _dm_get_status() invokes the status() method for each device mapper table, which writes a string to the supplied buffer as output. The buffer might contain multiple strings concatenated together. If there is not enough space available, the string is truncated and a termination character is put at the end. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- drivers/md/dm-builtin.c | 13 +++++++ drivers/md/dm-core.h | 5 +++ drivers/md/dm.c | 71 +++++++++++++++++++++++++++++++++++ include/linux/device-mapper.h | 3 ++ 4 files changed, 92 insertions(+)