Re: [PATCH v5] Introduce support for Systems Management Driver over WMI for Dell Systems

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux