| Message ID | 20210715230850.389961-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: think-lmi: add debug_cmd | expand |
Hi Mark, On 7/16/21 1:08 AM, Mark Pearson wrote: > Many Lenovo BIOS's support the ability to send a debug command which > is useful for debugging and testing unreleased or early features. > Adding support for this feature. > > Also moved the pending_reboot node under attributes dir where it should > correctly live. Got that wrong in my last commit and realised as I was > updating the documentation for debug_cmd > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> Thank you for the patch, I'm not entirely sure if we want this in its current form though, isn't this new debug_cmd file not something which would be better under debugfs ? Or maybe have it only show up if a module parameter is passed to enable it ? Note that both have implications wrt secureboot. debugfs is only writeable when secureboot is disabled; and ATM module options are not protected by secureboot, but at least in Fedora we would actually like to change that in the future. Anyways, lets discuss this a bit further and then merge something for this later. The pending_reboot move is good to have. You really should have submitted this as a separate patch. Hint as soon as you write "Also ...", or e.g. "This commit does the following 3 things: 1... 2... 3..." or some such in the commit message that is a hint that you should split the patch. I've split out the pending_reboot changes (and also updated the remove path which you left unchanged) now. While looking into the remove path, I also found a couple of other probe-failure related issues so I'll be sending out a patch-set with a few small fixes (including the pending_reboot move) soon. Regards, Hans > --- > .../testing/sysfs-class-firmware-attributes | 11 +++ > drivers/platform/x86/think-lmi.c | 77 ++++++++++++++++++- > drivers/platform/x86/think-lmi.h | 1 + > 3 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes > index 3348bf80a37c..db0aa2939efc 100644 > --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes > +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes > @@ -272,3 +272,14 @@ Description: > > Note that any changes to this attribute requires a reboot > for changes to take effect. > + > +What: /sys/class/firmware-attributes/*/attributes/debug_cmd > +Date: July 2021 > +KernelVersion: 5.14 > +Contact: Mark Pearson <markpearson@lenovo.com> > +Description: > + This write only attribute can be used to send debug commands to the BIOS. > + This should only be used when recommended by the BIOS vendor. Vendors may > + use it to enable extra debug attributes or BIOS features for testing purposes. > + > + Note that any changes to this attribute requires a reboot for changes to take effect. > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 64dcec53a7a0..d58d4b155139 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -116,6 +116,14 @@ > */ > #define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B" > > +/* > + * Name: > + * Lenovo_DebugCmdGUID > + * Description > + * Debug entry GUID method for entering debug commands to the BIOS > + */ > +#define LENOVO_DEBUG_CMD_GUID "7FF47003-3B6C-4E5E-A227-E979824A85D1" > + > #define TLMI_POP_PWD (1 << 0) > #define TLMI_PAP_PWD (1 << 1) > #define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj) > @@ -660,6 +668,63 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute * > > static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot); > > +/* ---- Debug interface--------------------------------------------------------- */ > +static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + char *set_str = NULL, *new_setting = NULL; > + char *auth_str = NULL; > + char *p; > + int ret; > + > + if (!tlmi_priv.can_debug_cmd) > + return -EOPNOTSUPP; > + > + new_setting = kstrdup(buf, GFP_KERNEL); > + if (!new_setting) > + return -ENOMEM; > + > + /* Strip out CR if one is present */ > + p = strchrnul(new_setting, '\n'); > + *p = '\0'; > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) { > + auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;", > + tlmi_priv.pwd_admin->password, > + encoding_options[tlmi_priv.pwd_admin->encoding], > + tlmi_priv.pwd_admin->kbdlang); > + if (!auth_str) { > + ret = -ENOMEM; > + goto out; > + } > + } > + > + if (auth_str) > + set_str = kasprintf(GFP_KERNEL, "%s,%s", new_setting, auth_str); > + else > + set_str = kasprintf(GFP_KERNEL, "%s;", new_setting); > + if (!set_str) { > + ret = -ENOMEM; > + goto out; > + } > + > + ret = tlmi_simple_call(LENOVO_DEBUG_CMD_GUID, set_str); > + if (ret) > + goto out; > + > + if (!ret && !tlmi_priv.pending_changes) { > + tlmi_priv.pending_changes = true; > + /* let userland know it may need to check reboot pending again */ > + kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE); > + } > +out: > + kfree(auth_str); > + kfree(set_str); > + kfree(new_setting); > + return ret ?: count; > +} > +static struct kobj_attribute debug_cmd = __ATTR_WO(debug_cmd); > + > /* ---- Initialisation --------------------------------------------------------- */ > static void tlmi_release_attr(void) > { > @@ -681,6 +746,8 @@ static void tlmi_release_attr(void) > kobject_put(&tlmi_priv.pwd_power->kobj); > kset_unregister(tlmi_priv.authentication_kset); > sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr); > + if (tlmi_priv.can_debug_cmd) > + sysfs_remove_file(&tlmi_priv.class_dev->kobj, &debug_cmd.attr); > } > > static int tlmi_sysfs_init(void) > @@ -761,10 +828,15 @@ static int tlmi_sysfs_init(void) > goto fail_create_attr; > > /* Create global sysfs files */ > - ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr); > + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr); > if (ret) > goto fail_create_attr; > > + if (tlmi_priv.can_debug_cmd) { > + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr); > + if (ret) > + goto fail_create_attr; > + } > return ret; > > fail_create_attr: > @@ -796,6 +868,9 @@ static int tlmi_analyze(void) > if (wmi_has_guid(LENOVO_BIOS_PASSWORD_SETTINGS_GUID)) > tlmi_priv.can_get_password_settings = true; > > + if (wmi_has_guid(LENOVO_DEBUG_CMD_GUID)) > + tlmi_priv.can_debug_cmd = true; > + > /* > * Try to find the number of valid settings of this machine > * and use it to create sysfs attributes. > diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h > index eb598846628a..f8e26823075f 100644 > --- a/drivers/platform/x86/think-lmi.h > +++ b/drivers/platform/x86/think-lmi.h > @@ -61,6 +61,7 @@ struct think_lmi { > bool can_set_bios_password; > bool can_get_password_settings; > bool pending_changes; > + bool can_debug_cmd; > > struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT]; > struct device *class_dev; >
Thanks for the review Hans On 2021-07-17 9:59 a.m., Hans de Goede wrote: > Hi Mark, > > On 7/16/21 1:08 AM, Mark Pearson wrote: >> Many Lenovo BIOS's support the ability to send a debug command which >> is useful for debugging and testing unreleased or early features. >> Adding support for this feature. >> >> Also moved the pending_reboot node under attributes dir where it should >> correctly live. Got that wrong in my last commit and realised as I was >> updating the documentation for debug_cmd >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> > > Thank you for the patch, I'm not entirely sure if we want this in its > current form though, isn't this new debug_cmd file not something which > would be better under debugfs ? Or maybe have it only show up if > a module parameter is passed to enable it ? > > Note that both have implications wrt secureboot. debugfs is only > writeable when secureboot is disabled; and ATM module options are > not protected by secureboot, but at least in Fedora we would actually > like to change that in the future. > > Anyways, lets discuss this a bit further and then merge something > for this later. > I wasn't sure about debugfs but did consider it. It seemed to be adding a whole extra layer. I'm happy to do that if that's what is recommended but not having it available with secureboot could be annoying for users. This feature is mostly used internally (which is fine) but we have had occasions where we've used it with customers; and with the WMI interface it's usually the enterprise customers who do have secure boot enabled. I'd like to avoid that limitation if possible. > The pending_reboot move is good to have. You really should have > submitted this as a separate patch. Hint as soon as you write > "Also ...", or e.g. "This commit does the following 3 things: > 1... 2... 3..." or some such in the commit message that is a hint > that you should split the patch. > > I've split out the pending_reboot changes (and also updated the > remove path which you left unchanged) now. While looking into > the remove path, I also found a couple of other probe-failure > related issues so I'll be sending out a patch-set with a > few small fixes (including the pending_reboot move) soon. Yeah - apologies. It was such a tiny change that I was being lazy. Thanks you for splitting them out but if you're busy let me know and I'll happily clean that myself. I had missed the remove part completely. Thanks Mark
Hi Mark, On 7/19/21 2:46 PM, Mark Pearson wrote: > Thanks for the review Hans > > On 2021-07-17 9:59 a.m., Hans de Goede wrote: >> Hi Mark, >> >> On 7/16/21 1:08 AM, Mark Pearson wrote: >>> Many Lenovo BIOS's support the ability to send a debug command which >>> is useful for debugging and testing unreleased or early features. >>> Adding support for this feature. >>> >>> Also moved the pending_reboot node under attributes dir where it should >>> correctly live. Got that wrong in my last commit and realised as I was >>> updating the documentation for debug_cmd >>> >>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> >> Thank you for the patch, I'm not entirely sure if we want this in its >> current form though, isn't this new debug_cmd file not something which >> would be better under debugfs ? Or maybe have it only show up if >> a module parameter is passed to enable it ? >> >> Note that both have implications wrt secureboot. debugfs is only >> writeable when secureboot is disabled; and ATM module options are >> not protected by secureboot, but at least in Fedora we would actually >> like to change that in the future. >> >> Anyways, lets discuss this a bit further and then merge something >> for this later. >> > I wasn't sure about debugfs but did consider it. It seemed to be adding a whole extra layer. I'm happy to do that if that's what is recommended but not having it available with secureboot could be annoying for users. I agree that adding debugfs support just for a single file seems a bit overkill. > This feature is mostly used internally (which is fine) but we have had occasions where we've used it with customers; and with the WMI interface it's usually the enterprise customers who do have secure boot enabled. I'd like to avoid that limitation if possible. So given that using debugfs seems a bit overkill and it has issues with selinux I think that it might be best to hide this file behind a module option (and mention that in the docs, or maybe not document it at all?). At least for now kernel commandline options an be set without breaking secureboot and even if secureboot ever starts verifying the cmdline then we can always use a /etc/modprobe.d/*.conf file to specify the option instead of passing it through the kernel cmdline. Would using a module option to enable the debug_cmd file work for you ? Regards, Hans
Hi Hans On 2021-07-29 1:34 p.m., Hans de Goede wrote: > Hi Mark, > > On 7/19/21 2:46 PM, Mark Pearson wrote: >> Thanks for the review Hans >> >> On 2021-07-17 9:59 a.m., Hans de Goede wrote: >>> Hi Mark, >>> >>> On 7/16/21 1:08 AM, Mark Pearson wrote: >>>> Many Lenovo BIOS's support the ability to send a debug command >>>> which is useful for debugging and testing unreleased or early >>>> features. Adding support for this feature. >>>> >>>> Also moved the pending_reboot node under attributes dir where >>>> it should correctly live. Got that wrong in my last commit and >>>> realised as I was updating the documentation for debug_cmd >>>> >>>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >>> >>> Thank you for the patch, I'm not entirely sure if we want this in >>> its current form though, isn't this new debug_cmd file not >>> something which would be better under debugfs ? Or maybe have it >>> only show up if a module parameter is passed to enable it ? >>> >>> Note that both have implications wrt secureboot. debugfs is only >>> writeable when secureboot is disabled; and ATM module options >>> are not protected by secureboot, but at least in Fedora we would >>> actually like to change that in the future. >>> >>> Anyways, lets discuss this a bit further and then merge >>> something for this later. >>> >> I wasn't sure about debugfs but did consider it. It seemed to be >> adding a whole extra layer. I'm happy to do that if that's what is >> recommended but not having it available with secureboot could be >> annoying for users. > > I agree that adding debugfs support just for a single file seems a > bit overkill. > >> This feature is mostly used internally (which is fine) but we have >> had occasions where we've used it with customers; and with the WMI >> interface it's usually the enterprise customers who do have secure >> boot enabled. I'd like to avoid that limitation if possible. > > So given that using debugfs seems a bit overkill and it has issues > with selinux I think that it might be best to hide this file behind a > module option (and mention that in the docs, or maybe not document it > at all?). > > At least for now kernel commandline options an be set without > breaking secureboot and even if secureboot ever starts verifying the > cmdline then we can always use a /etc/modprobe.d/*.conf file to > specify the option instead of passing it through the kernel cmdline. > > Would using a module option to enable the debug_cmd file work for you > ? > That sounds very sensible to me - totally happy to do that. I'll get that implemented when I'm back from PTO and send as an update. Thanks! Mark
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes index 3348bf80a37c..db0aa2939efc 100644 --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes @@ -272,3 +272,14 @@ Description: Note that any changes to this attribute requires a reboot for changes to take effect. + +What: /sys/class/firmware-attributes/*/attributes/debug_cmd +Date: July 2021 +KernelVersion: 5.14 +Contact: Mark Pearson <markpearson@lenovo.com> +Description: + This write only attribute can be used to send debug commands to the BIOS. + This should only be used when recommended by the BIOS vendor. Vendors may + use it to enable extra debug attributes or BIOS features for testing purposes. + + Note that any changes to this attribute requires a reboot for changes to take effect. diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 64dcec53a7a0..d58d4b155139 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -116,6 +116,14 @@ */ #define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B" +/* + * Name: + * Lenovo_DebugCmdGUID + * Description + * Debug entry GUID method for entering debug commands to the BIOS + */ +#define LENOVO_DEBUG_CMD_GUID "7FF47003-3B6C-4E5E-A227-E979824A85D1" + #define TLMI_POP_PWD (1 << 0) #define TLMI_PAP_PWD (1 << 1) #define to_tlmi_pwd_setting(kobj) container_of(kobj, struct tlmi_pwd_setting, kobj) @@ -660,6 +668,63 @@ static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute * static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot); +/* ---- Debug interface--------------------------------------------------------- */ +static ssize_t debug_cmd_store(struct kobject *kobj, struct kobj_attribute *attr, + const char *buf, size_t count) +{ + char *set_str = NULL, *new_setting = NULL; + char *auth_str = NULL; + char *p; + int ret; + + if (!tlmi_priv.can_debug_cmd) + return -EOPNOTSUPP; + + new_setting = kstrdup(buf, GFP_KERNEL); + if (!new_setting) + return -ENOMEM; + + /* Strip out CR if one is present */ + p = strchrnul(new_setting, '\n'); + *p = '\0'; + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password[0]) { + auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;", + tlmi_priv.pwd_admin->password, + encoding_options[tlmi_priv.pwd_admin->encoding], + tlmi_priv.pwd_admin->kbdlang); + if (!auth_str) { + ret = -ENOMEM; + goto out; + } + } + + if (auth_str) + set_str = kasprintf(GFP_KERNEL, "%s,%s", new_setting, auth_str); + else + set_str = kasprintf(GFP_KERNEL, "%s;", new_setting); + if (!set_str) { + ret = -ENOMEM; + goto out; + } + + ret = tlmi_simple_call(LENOVO_DEBUG_CMD_GUID, set_str); + if (ret) + goto out; + + if (!ret && !tlmi_priv.pending_changes) { + tlmi_priv.pending_changes = true; + /* let userland know it may need to check reboot pending again */ + kobject_uevent(&tlmi_priv.class_dev->kobj, KOBJ_CHANGE); + } +out: + kfree(auth_str); + kfree(set_str); + kfree(new_setting); + return ret ?: count; +} +static struct kobj_attribute debug_cmd = __ATTR_WO(debug_cmd); + /* ---- Initialisation --------------------------------------------------------- */ static void tlmi_release_attr(void) { @@ -681,6 +746,8 @@ static void tlmi_release_attr(void) kobject_put(&tlmi_priv.pwd_power->kobj); kset_unregister(tlmi_priv.authentication_kset); sysfs_remove_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr); + if (tlmi_priv.can_debug_cmd) + sysfs_remove_file(&tlmi_priv.class_dev->kobj, &debug_cmd.attr); } static int tlmi_sysfs_init(void) @@ -761,10 +828,15 @@ static int tlmi_sysfs_init(void) goto fail_create_attr; /* Create global sysfs files */ - ret = sysfs_create_file(&tlmi_priv.class_dev->kobj, &pending_reboot.attr); + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr); if (ret) goto fail_create_attr; + if (tlmi_priv.can_debug_cmd) { + ret = sysfs_create_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr); + if (ret) + goto fail_create_attr; + } return ret; fail_create_attr: @@ -796,6 +868,9 @@ static int tlmi_analyze(void) if (wmi_has_guid(LENOVO_BIOS_PASSWORD_SETTINGS_GUID)) tlmi_priv.can_get_password_settings = true; + if (wmi_has_guid(LENOVO_DEBUG_CMD_GUID)) + tlmi_priv.can_debug_cmd = true; + /* * Try to find the number of valid settings of this machine * and use it to create sysfs attributes. diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h index eb598846628a..f8e26823075f 100644 --- a/drivers/platform/x86/think-lmi.h +++ b/drivers/platform/x86/think-lmi.h @@ -61,6 +61,7 @@ struct think_lmi { bool can_set_bios_password; bool can_get_password_settings; bool pending_changes; + bool can_debug_cmd; struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT]; struct device *class_dev;
Many Lenovo BIOS's support the ability to send a debug command which is useful for debugging and testing unreleased or early features. Adding support for this feature. Also moved the pending_reboot node under attributes dir where it should correctly live. Got that wrong in my last commit and realised as I was updating the documentation for debug_cmd Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- .../testing/sysfs-class-firmware-attributes | 11 +++ drivers/platform/x86/think-lmi.c | 77 ++++++++++++++++++- drivers/platform/x86/think-lmi.h | 1 + 3 files changed, 88 insertions(+), 1 deletion(-)