| Message ID | 20230312024635.518769-1-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] platform/x86: think-lmi: add missing type attribute | expand |
On Sat, Mar 11, 2023 at 09:46:34PM -0500, Mark Pearson wrote: > This driver was missing the mandatory type attribute...oops. > > Add it in along with logic to determine whether the attribute is an > enumeration type or a string by parsing the possible_values attribute. > > Some platforms (and some attributes) don't return possible_values so to > prevent trying to scan NULL strings mark these as "N/A". > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460 Afaik Fixes: is only for references to commits. Instead a Reported-by/Link would be better. > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> > --- > drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++--- > drivers/platform/x86/think-lmi.h | 6 ++++++ > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 86b33b74519b..495a5e045069 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute > { > struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); > > - if (!tlmi_priv.can_get_bios_selections) > - return -EOPNOTSUPP; > - > return sysfs_emit(buf, "%s\n", setting->possible_values); > } > > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); > + > + return sysfs_emit(buf, "%s\n", > + setting->type == TLMI_ENUMERATION ? "enumeration" : "string"); > +} > + > static ssize_t current_value_store(struct kobject *kobj, > struct kobj_attribute *attr, > const char *buf, size_t count) > @@ -1036,10 +1042,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); > > static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); > > +static struct kobj_attribute attr_type = __ATTR_RO(type); > + > static struct attribute *tlmi_attrs[] = { > &attr_displ_name.attr, > &attr_current_val.attr, > &attr_possible_values.attr, > + &attr_type.attr, > NULL > }; > > @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void) > pr_info("Error retrieving possible values for %d : %s\n", > i, setting->display_name); > } > + /* If we don't have a possible value mark as N/A */ > + if (!setting->possible_values) { > + setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL); kmalloc() can fail. > + sprintf(setting->possible_values, "N/A"); This writes the '\0' out of bounds? kmalloc() and sprintf() could be replaced with kstrdup(). Instead of having to do allocations, check for failure, worry about how sysfs_emit() will handle the NULL it would be easier to just check of NULL inside possible_values_show() and fall back to N/A there. > + } > + /* Figure out what setting type is as BIOS does not return this */ > + if (strchr(setting->possible_values, ',')) possible_values could be NULL if the sprintf would not have dereferenced it before. > + setting->type = TLMI_ENUMERATION; > + else > + setting->type = TLMI_STRING; > + Is it worth introducing a new enum and field in struct tlmi_attr_setting? The check could also be done directly in type_show(). (with a NULL-check). > kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); > tlmi_priv.setting[i] = setting; > kfree(item); > diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h > index 4daba6151cd6..b76edcb9b1af 100644 > --- a/drivers/platform/x86/think-lmi.h > +++ b/drivers/platform/x86/think-lmi.h > @@ -27,6 +27,11 @@ enum level_option { > TLMI_LEVEL_MASTER, > }; > > +enum attr_type { > + TLMI_ENUMERATION, > + TLMI_STRING, > +}; > + > /* password configuration details */ > struct tlmi_pwdcfg_core { > uint32_t password_mode; > @@ -73,6 +78,7 @@ struct tlmi_attr_setting { > int index; > char display_name[TLMI_SETTINGS_MAXLEN]; > char *possible_values; > + enum attr_type type; > }; > > struct think_lmi { > -- > 2.39.1 >
Apologies for the duplication, I forgot to set email format as text so it got bounced by the mailing list. Resending. Mark On Sat, Mar 11, 2023, at 10:44 PM, Mark Pearson wrote: > Thanks Thomas > > On Sat, Mar 11, 2023, at 10:33 PM, Thomas Weißschuh wrote: >> On Sat, Mar 11, 2023 at 09:46:34PM -0500, Mark Pearson wrote: >> > This driver was missing the mandatory type attribute...oops. >> > >> > Add it in along with logic to determine whether the attribute is an >> > enumeration type or a string by parsing the possible_values attribute. >> > >> > Some platforms (and some attributes) don't return possible_values so to >> > prevent trying to scan NULL strings mark these as "N/A". >> > >> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460 >> >> Afaik Fixes: is only for references to commits. >> Instead a Reported-by/Link would be better. > Ah - thanks. My bad. > >> >> > Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> >> > --- >> > drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++--- >> > drivers/platform/x86/think-lmi.h | 6 ++++++ >> > 2 files changed, 29 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> > index 86b33b74519b..495a5e045069 100644 >> > --- a/drivers/platform/x86/think-lmi.c >> > +++ b/drivers/platform/x86/think-lmi.c >> > @@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute >> > { >> > struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); >> > >> > - if (!tlmi_priv.can_get_bios_selections) >> > - return -EOPNOTSUPP; >> > - >> > return sysfs_emit(buf, "%s\n", setting->possible_values); >> > } >> > >> > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, >> > + char *buf) >> > +{ >> > + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); >> > + >> > + return sysfs_emit(buf, "%s\n", >> > + setting->type == TLMI_ENUMERATION ? "enumeration" : "string"); >> > +} >> > + >> > static ssize_t current_value_store(struct kobject *kobj, >> > struct kobj_attribute *attr, >> > const char *buf, size_t count) >> > @@ -1036,10 +1042,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); >> > >> > static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); >> > >> > +static struct kobj_attribute attr_type = __ATTR_RO(type); >> > + >> > static struct attribute *tlmi_attrs[] = { >> > &attr_displ_name.attr, >> > &attr_current_val.attr, >> > &attr_possible_values.attr, >> > + &attr_type.attr, >> > NULL >> > }; >> > >> > @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void) >> > pr_info("Error retrieving possible values for %d : %s\n", >> > i, setting->display_name); >> > } >> > + /* If we don't have a possible value mark as N/A */ >> > + if (!setting->possible_values) { >> > + setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL); >> >> kmalloc() can fail. >> >> > + sprintf(setting->possible_values, "N/A"); >> >> This writes the '\0' out of bounds? >> >> kmalloc() and sprintf() could be replaced with kstrdup(). >> >> Instead of having to do allocations, check for failure, worry about how >> sysfs_emit() will handle the NULL it would be easier to just check of >> NULL inside possible_values_show() and fall back to N/A there. > Good point - that would be better. I'll update. > >> >> > + } >> > + /* Figure out what setting type is as BIOS does not return this */ >> > + if (strchr(setting->possible_values, ',')) >> >> possible_values could be NULL if the sprintf would not have dereferenced >> it before. > Agreed. This was part of the reason I'd put in the N/A to cover for that case (so it should never be NULL). But I'll revisit this. > >> >> > + setting->type = TLMI_ENUMERATION; >> > + else >> > + setting->type = TLMI_STRING; >> > + >> >> Is it worth introducing a new enum and field in struct >> tlmi_attr_setting? >> The check could also be done directly in type_show(). >> (with a NULL-check). > Ack, this makes sense. I'll look at doing that. > > Many thanks for the review > Mark
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 86b33b74519b..495a5e045069 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -941,12 +941,18 @@ static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute { struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); - if (!tlmi_priv.can_get_bios_selections) - return -EOPNOTSUPP; - return sysfs_emit(buf, "%s\n", setting->possible_values); } +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj); + + return sysfs_emit(buf, "%s\n", + setting->type == TLMI_ENUMERATION ? "enumeration" : "string"); +} + static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) @@ -1036,10 +1042,13 @@ static struct kobj_attribute attr_possible_values = __ATTR_RO(possible_values); static struct kobj_attribute attr_current_val = __ATTR_RW_MODE(current_value, 0600); +static struct kobj_attribute attr_type = __ATTR_RO(type); + static struct attribute *tlmi_attrs[] = { &attr_displ_name.attr, &attr_current_val.attr, &attr_possible_values.attr, + &attr_type.attr, NULL }; @@ -1424,6 +1433,17 @@ static int tlmi_analyze(void) pr_info("Error retrieving possible values for %d : %s\n", i, setting->display_name); } + /* If we don't have a possible value mark as N/A */ + if (!setting->possible_values) { + setting->possible_values = kmalloc(strlen("N/A"), GFP_KERNEL); + sprintf(setting->possible_values, "N/A"); + } + /* Figure out what setting type is as BIOS does not return this */ + if (strchr(setting->possible_values, ',')) + setting->type = TLMI_ENUMERATION; + else + setting->type = TLMI_STRING; + kobject_init(&setting->kobj, &tlmi_attr_setting_ktype); tlmi_priv.setting[i] = setting; kfree(item); diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h index 4daba6151cd6..b76edcb9b1af 100644 --- a/drivers/platform/x86/think-lmi.h +++ b/drivers/platform/x86/think-lmi.h @@ -27,6 +27,11 @@ enum level_option { TLMI_LEVEL_MASTER, }; +enum attr_type { + TLMI_ENUMERATION, + TLMI_STRING, +}; + /* password configuration details */ struct tlmi_pwdcfg_core { uint32_t password_mode; @@ -73,6 +78,7 @@ struct tlmi_attr_setting { int index; char display_name[TLMI_SETTINGS_MAXLEN]; char *possible_values; + enum attr_type type; }; struct think_lmi {
This driver was missing the mandatory type attribute...oops. Add it in along with logic to determine whether the attribute is an enumeration type or a string by parsing the possible_values attribute. Some platforms (and some attributes) don't return possible_values so to prevent trying to scan NULL strings mark these as "N/A". Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216460 Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca> --- drivers/platform/x86/think-lmi.c | 26 +++++++++++++++++++++++--- drivers/platform/x86/think-lmi.h | 6 ++++++ 2 files changed, 29 insertions(+), 3 deletions(-)