Messages in this thread | | | Subject | Re: [PATCH v5] Introduce support for Systems Management Driver over WMI for Dell Systems | From | Hans de Goede <> | Date | Fri, 9 Oct 2020 09:46:52 +0200 |
| |
Hi,
On 10/1/20 9:37 PM, Limonciello, Mario wrote: >>> + >>> + if (!capable(CAP_SYS_ADMIN)) >>> + return -EPERM; >> >> Sorry for not addressing this during earlier reviews, but why is this >> check here. Is read-only access to the settings by normal users >> considered harmful ? >> > > The best answer I can give is that this is exposing data to a user that > previously they would have needed either physical access or root access > to see. And even if they had physical access they may have needed a > BIOS admin password to get "into" setup. Exposing it directly to everyone > subverts the previous access limitations to the data. > > Some of the settings are certainly more sensitive than others, so I don't > feel it's appropriate for the kernel to perform this arbitration.
I see, IMHO it would be better to just set the file permissions for current_value to 600 then, then it will be much clearer to users why they are getting -EPERM. This is e.g. also done for some of the more sensitive DMI strings like e.g. serial-numbers:
[hans@x1 ~]$ ls -l /sys/class/dmi/id/board_serial -r-------- 1 root root 4096 Oct 9 09:36 /sys/class/dmi/id/board_serial
You can do this by changing:
__ATTR_RW(current_value);
to:
__ATTR_RW_MODE(current_value, 0600);
>>> +static int validate_enumeration_input(int instance_id, const char *buf) >>> +{ >>> + char *options, *tmp, *p; >>> + int ret = -EINVAL; >>> + >>> + options = tmp = >> kstrdup(wmi_priv.enumeration_data[instance_id].possible_values, >>> + GFP_KERNEL); >>> + if (!options) >>> + return -ENOMEM; >>> + >>> + while ((p = strsep(&options, ";")) != NULL) { >>> + if (!*p) >>> + continue; >>> + if (!strncasecmp(p, buf, strlen(p))) { >> >> So using strncasecmp here is usually done to get rid of the '\n' but it >> is a bit finicky and as such you got it wrong here, of say "Enabled" >> is a valid value and the user passes "EnabledFooBar" then this >> will get accepted because the first 7 chars match. Since you >> are already dealing with the \n in the caller you can just drop the >> "n" part of the strncasecmp to fix this. >> >> Also are you sure you want a strcasecmp here ? That makes the compare >> case-insensitive. So IOW that means that enabled and ENABLED are also >> acceptable. > > That's correct, the firmware will interpret ENABLED and enabled as the same > thing. It will also do further validation of the input.
Ok, strcasecmp it is then.
Regards,
Hans
| |