Message-ID: <DM6PR19MB26368CC7E63EB64C380C9F63FA300@DM6PR19MB2636.namprd19.prod.outlook.com>
Date:   Thu, 1 Oct 2020 19:37:57 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        mark gross <mgross@...ux.intel.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: RE: [PATCH v5] Introduce support for Systems Management Driver over
 WMI for Dell Systems

> > +
> > +	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.

> > +	instance_id = get_enumeration_instance_id(kobj);
> 
> Can we please move the error return to directly below
> the function call and propagate the error please?
> So make it:
> 
> 	instance_id = get_enumeration_instance_id(kobj);
> 	if (instance_id < 0)
> 		return instance_id;
> 
> 	/* Non error path code here */
> 	return sprintf(buf, "%s\n",
> wmi_priv.enumeration_data[instance_id].current_value);
> 
> This is the normal way of error handling in the kernel, the
> if (success) method gets unwieldly with nested success checks:
> 
> 	if (success) {
> 		...
> 		if (success) {
> 			...
> 			if (success) {
> 				...
> 
> > +	if (instance_id >= 0) {
> > +		union acpi_object *obj;
> > +
> > +		/* need to use specific instance_id and guid combination to get
> right data */
> > +		obj = get_wmiobj_pointer(instance_id,
> DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
> > +		if (!obj)
> > +			return -AE_ERROR;
> 
> -AE_ERROR is not a standard errno.h error code, so it should not be used here.

(There should probably be some cleanup to other drivers too that use this)

> 
> > +		strlcpy_attr(wmi_priv.enumeration_data[instance_id].current_value,
> > +		       obj->package.elements[CURRENT_VAL].string.pointer);
> > +		kfree(obj);
> > +		return sprintf(buf, "%s\n",
> wmi_priv.enumeration_data[instance_id].current_value);
> 
> Why do the strcpy at all, why not directly sprintf the obj-
> >package.elements[CURRENT_VAL].string.pointer ?
> the only advantage I see to the strcpy is not overflowing buf.
> 
> How about this instead:
> 
> 	union acpi_object *obj;
> 	ssize_t ret;
> 
> 	instance_id = get_enumeration_instance_id(kobj);
> 	if (instance_id < 0)
> 		return instance_id;
> 
> 	obj = get_wmiobj_pointer(instance_id,
> DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
> 	if (!obj)
> 		return -EIO;
> 
> 	sprintf(buf, "%s\n",
> 	ret = snprintf(buf, PAGE_SIZE, "%s\n", obj-
> >package.elements[CURRENT_VAL].string.pointer);
> 	kfree(obj);
> 	return ret;
> }
> 
> Then we can completely drop the current_value member from struct
> enumeration_data.
> 
> > +	}
> > +	return -EIO;
> > +}
> > +
> > +/**
> > + * validate_enumeration_input() - Validate input of current_value against
> possible values
> > + * @instance_id: The instance on which input is validated
> > + * @buf: Input value
> > + **/
> 
> The kernel typically terminates kerneldoc comments with a standard '*/'
> terminator (not '**/'). Note this comment applies to all kerneldoc comments
> in the patch.
> 
> > +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.

$ echo "enabled" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value 
enabled
$ echo "enabledfoobar" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value 
enabledfoobar
tee: /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value: Input/output error
$ echo "EnABLED" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value 
EnABLED
$ sudo cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
Enabled

> 
> Maybe also check against min/max password lenght here?
> 
> I guess that is mostly important when changing the password, since
> if the length is not valid then it is just a wrong password.
> 
> So I guess it is fine to only check for not exceeding MAX_BUF here.

Yes, the firmware will do this check.

Powered by blists - more mailing lists