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
