| Message ID | 9f94be9c32887aacdcba75bd6a3902d0350eb987.1521482296.git.jonas.rabenstein@studium.uni-erlangen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > Allow modification of the shadow mbr. If the shadow mbr is not marked as > done, this data will be presented read only as the device content. Only > after marking the shadow mbr as done and unlocking a locking range the > actual content is accessible. I hate doing this as an ioctls. Can we make this a sysfs binary file so that people can use dd or cat to write the shadow mbr?
On Mon, Mar 19, 2018 at 08:52:24PM +0100, Christoph Hellwig wrote: > On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > > Allow modification of the shadow mbr. If the shadow mbr is not marked as > > done, this data will be presented read only as the device content. Only > > after marking the shadow mbr as done and unlocking a locking range the > > actual content is accessible. > > I hate doing this as an ioctls. Can we make this a sysfs binary file > so that people can use dd or cat to write the shadow mbr? I already thought about providing a sysfs interface for all that instead of using ioctls. But as I am pretty new to kernel programming I do not have all the required insight. Especially, as writing the mbr requires the sed-opal password I am unsure how a clean sysfs interface to provide the password together with a simple dd would look like. Moreover I already have a patch that changes the 'void *data' argument to setup_opal_dev to a kobject pointer. As far as I know, this is the first step to get into the sysfs hierarchy. But as I do not have access to an NVMe drive and have no idea about its implementation, this change works only for the scsi side. In other words, if someone could hint me in the right direction, I would be glad to (re)implement the ioctl interface for sysfs. Moreover, this would allow to export some additional information like the current state of the device (is it looke, is sed-opal enabled, whats the current state of mbr, etc.). - Jonas
On Tue, Mar 20, 2018 at 10:36:04AM +0100, Jonas Rabenstein wrote: > On Mon, Mar 19, 2018 at 08:52:24PM +0100, Christoph Hellwig wrote: > > On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > > > Allow modification of the shadow mbr. If the shadow mbr is not marked as > > > done, this data will be presented read only as the device content. Only > > > after marking the shadow mbr as done and unlocking a locking range the > > > actual content is accessible. > > > > I hate doing this as an ioctls. Can we make this a sysfs binary file > > so that people can use dd or cat to write the shadow mbr? > I already thought about providing a sysfs interface for all that instead > of using ioctls. But as I am pretty new to kernel programming I do not > have all the required insight. Especially, as writing the mbr requires > the sed-opal password I am unsure how a clean sysfs interface to provide > the password together with a simple dd would look like. > Moreover I already have a patch that changes the 'void *data' argument > to setup_opal_dev to a kobject pointer. As far as I know, this is the > first step to get into the sysfs hierarchy. But as I do not have access > to an NVMe drive and have no idea about its implementation, this change > works only for the scsi side. Post what you have as an RFC (review for comment) and I will test for the NVMe side, and or start a port for NVMe. It doesn't have to be perfect since you're sending it out as RFC. It's just a base for us to test/look at to see if we still like the sysfs way.
On Tue, Mar 20, 2018 at 04:09:08PM -0600, Scott Bauer wrote: > On Tue, Mar 20, 2018 at 10:36:04AM +0100, Jonas Rabenstein wrote: > > On Mon, Mar 19, 2018 at 08:52:24PM +0100, Christoph Hellwig wrote: > > > On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > > > > Allow modification of the shadow mbr. If the shadow mbr is not marked as > > > > done, this data will be presented read only as the device content. Only > > > > after marking the shadow mbr as done and unlocking a locking range the > > > > actual content is accessible. > > > > > > I hate doing this as an ioctls. Can we make this a sysfs binary file > > > so that people can use dd or cat to write the shadow mbr? > > I already thought about providing a sysfs interface for all that instead > > of using ioctls. But as I am pretty new to kernel programming I do not > > have all the required insight. Especially, as writing the mbr requires > > the sed-opal password I am unsure how a clean sysfs interface to provide > > the password together with a simple dd would look like. > > Moreover I already have a patch that changes the 'void *data' argument > > to setup_opal_dev to a kobject pointer. As far as I know, this is the > > first step to get into the sysfs hierarchy. But as I do not have access > > to an NVMe drive and have no idea about its implementation, this change > > works only for the scsi side. > > Post what you have as an RFC (review for comment) and I will test for the NVMe > side, and or start a port for NVMe. It doesn't have to be perfect since you're > sending it out as RFC. It's just a base for us to test/look at to see if we > still like the sysfs way. Seems, like I failed to make my point in the previous message. I do not have more than adding a directory 'sed_opal' in sysfs for each sed-opal enabled disk. But that directory is completely empty. My further plans are, to fill up that directory with some public info like the one that gets printed by a call to TCG's 'sedutil-cli --query' command. The interesting part - where I am clueless how to achieve it - would be to have a binary sysfs attribute (like mentioned by Christoph) for the features (like shadow mbr) requiring some kind of authentication. Until there is any (good) idea, I do not think it is time for an RFC? -- Jonas
On Thu, Mar 29, 2018 at 07:30:02PM +0200, Jonas Rabenstein wrote: > Hi, > On Wed, Mar 21, 2018 at 02:43:21AM +0100, Jonas Rabenstein wrote: > > On Tue, Mar 20, 2018 at 04:09:08PM -0600, Scott Bauer wrote: > > > On Tue, Mar 20, 2018 at 10:36:04AM +0100, Jonas Rabenstein wrote: > > > > On Mon, Mar 19, 2018 at 08:52:24PM +0100, Christoph Hellwig wrote: > > > > > On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > > > > > I hate doing this as an ioctls. Can we make this a sysfs binary file > > > > > so that people can use dd or cat to write the shadow mbr? > > > > I already thought about providing a sysfs interface for all that instead > > > > of using ioctls. But as I am pretty new to kernel programming I do not > > > > have all the required insight. Especially, as writing the mbr requires > > > > the sed-opal password I am unsure how a clean sysfs interface to provide > > > > the password together with a simple dd would look like. > Just wanted to ask, how to proceed with those patches/what I should do. > Using sysfs instead of an ioctl is probably easier to use from userspace > _if_ there is a good way to provide the password - which I do not know > of :( > If nobody else could think of a solution, shall writes to the shadow mbr > remain unsupported? > > I'ld really appreciate feedback and possible solutions, > Jonas Yeah, having to autheticate to write the MBR is a real bummer. Theoretically you could dd a the pw struct + the shador MBR into sysfs. But that's a pretty disgusting hack just to use sysfs. The other method I thought of was to authenticate via ioctl then write via sysfs. We already save the PW in-kernel for unlocks, so perhaps we can re-use the save-for-unlock to do shadow MBR writes via sysfs? Re-using an already exposed ioctl for another purpose seems somewhat dangerous? In the sense that what if the user wants to write the smbr but doesn't want to unlock on suspends, or does not want their PW hanging around in the kernel. Overall I think the ioctl is still the best path forward due to the authentication problem. But am still willing to hear others opinions if they do have an idea. I can say yes to the Ioctl, but we really need Jens and Christophs Okay on it. I have some free time tomorrow to work on this, so let me goof with it tomorrow and over the weekend and I'll see if there is a sane way to get sysfs to work.
Hi, On Wed, Mar 21, 2018 at 02:43:21AM +0100, Jonas Rabenstein wrote: > On Tue, Mar 20, 2018 at 04:09:08PM -0600, Scott Bauer wrote: > > On Tue, Mar 20, 2018 at 10:36:04AM +0100, Jonas Rabenstein wrote: > > > On Mon, Mar 19, 2018 at 08:52:24PM +0100, Christoph Hellwig wrote: > > > > On Mon, Mar 19, 2018 at 07:36:50PM +0100, Jonas Rabenstein wrote: > > > > I hate doing this as an ioctls. Can we make this a sysfs binary file > > > > so that people can use dd or cat to write the shadow mbr? > > > I already thought about providing a sysfs interface for all that instead > > > of using ioctls. But as I am pretty new to kernel programming I do not > > > have all the required insight. Especially, as writing the mbr requires > > > the sed-opal password I am unsure how a clean sysfs interface to provide > > > the password together with a simple dd would look like. Just wanted to ask, how to proceed with those patches/what I should do. Using sysfs instead of an ioctl is probably easier to use from userspace _if_ there is a good way to provide the password - which I do not know of :( If nobody else could think of a solution, shall writes to the shadow mbr remain unsupported? I'ld really appreciate feedback and possible solutions, Jonas
On Thu, Mar 29, 2018 at 11:16:42AM -0600, Scott Bauer wrote: > Yeah, having to autheticate to write the MBR is a real bummer. Theoretically > you could dd a the pw struct + the shador MBR into sysfs. But that's > a pretty disgusting hack just to use sysfs. The other method I thought of > was to authenticate via ioctl then write via sysfs. We already save the PW > in-kernel for unlocks, so perhaps we can re-use the save-for-unlock to > do shadow MBR writes via sysfs? > > Re-using an already exposed ioctl for another purpose seems somewhat dangerous? > In the sense that what if the user wants to write the smbr but doesn't want to > unlock on suspends, or does not want their PW hanging around in the kernel. Well. If we would force the user to a two-step interaction, why not stay completely in sysfs? So instead of using the save-for-unlock ioctl, we could export each security provider( (AdminSP, UserSPX, ...) as a sysfs directory with appropriate files (e.g. mbr for AdminSP) as well as a 'unlock' file to store a users password for the specific locking space and a 'lock' file to remove the stored password on write to it. Of course, while this will prevent from reuse of the ioctl and stays within the same configuration method, the PW will still hang around in the kernel between 'lock' and 'unlock'. Another idea I just came across while writing this down: Instead of storing/releasing the password permanently with the 'unlock' and 'lock' files, those may be used to start/stop an authenticated session. To make it more clear what I mean: Each ioctl that requires authentication has a similar pattern: discovery0, start_session, <do_work>, end_session Instead of having the combination determined by the ioctl, the 'unlock' would do discovery0 and start_session while the 'lock' would do the end_session. The user is free to issue further commands with the appropriate write/reads to other files of the sysfs-directory. While this removes the requirement to store the key within kernel space, the open session handle may be used from everybody with permissions for read/write access to the sysfs-directory files. So this is not optimal as not only the user who provided the password will finally be able to use it. I already did some basic work to split of the session-information from the opal_dev struct (initially to reduce the memory-footprint of devices with currently no active opal-interaction). So I think, I could get a proof-of-concept of this approach within the next one or two weeks if there are no objections to the base idea. Thank you, Jonas
On Thu, Mar 29, 2018 at 08:27:30PM +0200, catchall@ghostav.ddnss.de wrote: > On Thu, Mar 29, 2018 at 11:16:42AM -0600, Scott Bauer wrote: > > Yeah, having to autheticate to write the MBR is a real bummer. Theoretically > > you could dd a the pw struct + the shador MBR into sysfs. But that's > > a pretty disgusting hack just to use sysfs. The other method I thought of > > was to authenticate via ioctl then write via sysfs. We already save the PW > > in-kernel for unlocks, so perhaps we can re-use the save-for-unlock to > > do shadow MBR writes via sysfs? > > > > Re-using an already exposed ioctl for another purpose seems somewhat dangerous? > > In the sense that what if the user wants to write the smbr but doesn't want to > > unlock on suspends, or does not want their PW hanging around in the kernel. > Well. If we would force the user to a two-step interaction, why not stay > completely in sysfs? So instead of using the save-for-unlock ioctl, we > could export each security provider( (AdminSP, UserSPX, ...) as a sysfs The Problem with this is Single user mode, where you can assign users to locking ranges. There would have to be a lot of dynamic changes of sysfs as users get added/removed, or added to LRs etc. It seems like we're trying mold something that already works fine into something that doesnt really work as we dig into the details. > directory with appropriate files (e.g. mbr for AdminSP) as well as a > 'unlock' file to store a users password for the specific locking space > and a 'lock' file to remove the stored password on write to it. > Of course, while this will prevent from reuse of the ioctl and > stays within the same configuration method, the PW will still hang > around in the kernel between 'lock' and 'unlock'. > > Another idea I just came across while writing this down: > Instead of storing/releasing the password permanently with the 'unlock' and > 'lock' files, those may be used to start/stop an authenticated session. > To make it more clear what I mean: Each ioctl that requires > authentication has a similar pattern: > discovery0, start_session, <do_work>, end_session > Instead of having the combination determined by the ioctl, the 'unlock' > would do discovery0 and start_session while the 'lock' would do the > end_session. The user is free to issue further commands with the > appropriate write/reads to other files of the sysfs-directory. > While this removes the requirement to store the key within kernel space, > the open session handle may be used from everybody with permissions for > read/write access to the sysfs-directory files. So this is not optimal > as not only the user who provided the password will finally be able to use > it. I generally like the idea of being able to run your abritrary opal commands, but: that's probably not going to work for the final reason you outlined. Even though it's root only access(to sysfs) we're breaking the authentication lower down by essentially allowing any opal command to be ran if you've somehow become root. The other issue with this is the session time out in opal. When we dispatch the commands in-kernel we're hammering them out 1-by-1. If the user needs to do an activatelsp, setuplr, etc. They do that with a new session. If someone starts the session and it times out it may be hard to figure out how to not get an SP_BUSY back from the controller. I've in the past just had to wipe my damn fw to get out of SP_BUSYs, but that could be due to the early implementations I was dealing with. > I already did some basic work to split of the session-information from > the opal_dev struct (initially to reduce the memory-footprint of devices with > currently no active opal-interaction). So I think, I could get a > proof-of-concept of this approach within the next one or two weeks if > there are no objections to the base idea. Sorry to ocme back a week later, but if you do have anything it would be at least interesting to see. I would still prefer the ioctl route, but will review and test any implementation people deem acceptable.
diff --git a/block/sed-opal.c b/block/sed-opal.c index 2c8baff8bf67..4549fa164e98 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -1492,6 +1492,54 @@ static int set_mbr_enable_disable(struct opal_dev *dev, void *data) return finalize_and_send(dev, parse_and_check_status); } +static int write_shadow_mbr(struct opal_dev *dev, void *data) +{ + struct opal_shadow_mbr *shadow = data; + const u8 __user *src; + u8 *dst; + size_t off; + u64 len; + int err = 0; + + /* FIXME: this is the maximum we can use for IO_BUFFER_LENGTH=2048. + * Instead of having a constant value, it would be nice to + * compute the actual value depending on IO_BUFFER_LENGTH + */ + len = 1950; + + /* do the actual transmission(s) */ + src = (u8 *) shadow->data; + for (off = 0 ; off < shadow->size; off += len) { + len = min(len, shadow->size - off); + + pr_debug("MBR: write bytes %zu+%llu/%llu\n", + off, len, shadow->size); + err = start_opal_cmd(dev, opaluid[OPAL_MBR], + opalmethod[OPAL_SET]); + add_token_u8(&err, dev, OPAL_STARTNAME); + add_token_u8(&err, dev, OPAL_WHERE); + add_token_u64(&err, dev, shadow->offset + off); + add_token_u8(&err, dev, OPAL_ENDNAME); + + add_token_u8(&err, dev, OPAL_STARTNAME); + add_token_u8(&err, dev, OPAL_VALUES); + dst = add_bytestring_header(&err, dev, len); + if (!dst) + break; + if (copy_from_user(dst, src + off, len)) + err = -EFAULT; + + add_token_u8(&err, dev, OPAL_ENDNAME); + if (err) + break; + + err = finalize_and_send(dev, parse_and_check_status); + if (err) + break; + } + return err; +} + static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid, struct opal_dev *dev) { @@ -2037,6 +2085,31 @@ static int opal_mbr_status(struct opal_dev *dev, struct opal_mbr_data *opal_mbr) return ret; } +static int opal_write_shadow_mbr(struct opal_dev *dev, + struct opal_shadow_mbr *info) +{ + const struct opal_step mbr_steps[] = { + { opal_discovery0, }, + { start_admin1LSP_opal_session, &info->key }, + { write_shadow_mbr, info }, + { end_opal_session, }, + { NULL, } + }; + int ret; + + if (info->size == 0) + return 0; + + if (!access_ok(VERIFY_READ, info->data, info->size)) + return -EINVAL; + + mutex_lock(&dev->dev_lock); + setup_opal_dev(dev, mbr_steps); + ret = next(dev); + mutex_unlock(&dev->dev_lock); + return ret; +} + static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk) { struct opal_suspend_data *suspend; @@ -2369,6 +2442,9 @@ int sed_ioctl(struct opal_dev *dev, unsigned int cmd, void __user *arg) case IOC_OPAL_MBR_STATUS: ret = opal_mbr_status(dev, p); break; + case IOC_OPAL_WRITE_SHADOW_MBR: + ret = opal_write_shadow_mbr(dev, p); + break; case IOC_OPAL_ERASE_LR: ret = opal_erase_locking_range(dev, p); break; diff --git a/include/linux/sed-opal.h b/include/linux/sed-opal.h index b38dc602cae3..cf08cdc13cbd 100644 --- a/include/linux/sed-opal.h +++ b/include/linux/sed-opal.h @@ -47,6 +47,7 @@ static inline bool is_sed_ioctl(unsigned int cmd) case IOC_OPAL_ENABLE_DISABLE_MBR: case IOC_OPAL_ERASE_LR: case IOC_OPAL_SECURE_ERASE_LR: + case IOC_OPAL_WRITE_SHADOW_MBR: case IOC_OPAL_MBR_STATUS: return true; } diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h index 0cb9890cdc04..8e84307f66d4 100644 --- a/include/uapi/linux/sed-opal.h +++ b/include/uapi/linux/sed-opal.h @@ -104,6 +104,13 @@ struct opal_mbr_data { __u8 __align[7]; }; +struct opal_shadow_mbr { + struct opal_key key; + const __u64 data; + __u64 offset; + __u64 size; +}; + #define IOC_OPAL_SAVE _IOW('p', 220, struct opal_lock_unlock) #define IOC_OPAL_LOCK_UNLOCK _IOW('p', 221, struct opal_lock_unlock) #define IOC_OPAL_TAKE_OWNERSHIP _IOW('p', 222, struct opal_key) @@ -117,5 +124,6 @@ struct opal_mbr_data { #define IOC_OPAL_ERASE_LR _IOW('p', 230, struct opal_session_info) #define IOC_OPAL_SECURE_ERASE_LR _IOW('p', 231, struct opal_session_info) #define IOC_OPAL_MBR_STATUS _IOW('p', 232, struct opal_mbr_data) +#define IOC_OPAL_WRITE_SHADOW_MBR _IOW('p', 233, struct opal_shadow_mbr) #endif /* _UAPI_SED_OPAL_H */
Allow modification of the shadow mbr. If the shadow mbr is not marked as done, this data will be presented read only as the device content. Only after marking the shadow mbr as done and unlocking a locking range the actual content is accessible. Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>