Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5] Introduce support for Systems Management Driver over
 WMI for Dell Systems
To:     "Limonciello, Mario" <Mario.Limonciello@dell.com>,
        Divya Bharathi <divya27392@gmail.com>,
        "dvhart@infradead.org" <dvhart@infradead.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@Dell.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        mark gross <mgross@linux.intel.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@dell.com>
References: <20200929025521.59573-1-divya.bharathi@dell.com>
 <7015e6d5-3c1e-e07e-572f-d5d47a9b0191@redhat.com>
 <DM6PR19MB26368CC7E63EB64C380C9F63FA300@DM6PR19MB2636.namprd19.prod.outlook.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <0c4a8f66-5261-5cf0-0cd4-6991d7406854@redhat.com>
Date:   Fri, 9 Oct 2020 09:46:52 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <DM6PR19MB26368CC7E63EB64C380C9F63FA300@DM6PR19MB2636.namprd19.prod.outlook.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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