linux-kernel - Re: [PATCH] Introduce support for Systems Management Driver over WMI for Dell Systems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71aebcf6-ecc5-72ce-e230-8bd25a294de8@redhat.com>
Date:   Mon, 14 Sep 2020 11:53:17 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Limonciello, Mario" <Mario.Limonciello@...l.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Divya Bharathi <divya27392@...il.com>,
        Darren Hart <dvhart@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: Re: [PATCH] Introduce support for Systems Management Driver over WMI
 for Dell Systems

Hi,

On 9/3/20 4:27 PM, Limonciello, Mario wrote:

<snip>

>>>> +     /* look up if user set a password for the requests */
>>>> +     current_password = get_current_password("Admin");
>>>> +     if (!current_password)
>>>> +             return -ENODEV;
>>>
>>> Can we instead of passing "Admin" and "System" to this function
>>> just have 2 separate get_current_admin_password and
>> get_current_system_password
>>> helpers and then drop the error handling ?
> 
> The error handling for -ENODEV is actually important in case a WMI driver
> was unbound.

I see and checking for that is good, but then please make it
explicit, rather then hiding it like this. As is the code suggests
to someone reading the code that the problem is a missing password not
the driver being unbound.

As I mentioned before, since the code clearly assume that
only 1 instance of each WMI GUID is present, it should move to
storing all its data into a shared global struct. Protected
by a single shared global mutex.

And then functions exposed through sysfs attributes can do:

	mutex_lock(&dell_wmi_bios_attr_mutex);

	if (!dell_wmi_bios_attr_data.bios_attr_wdev ||
	    !dell_wmi_bios_attr_data.password_attr_wdev) {
		mutex_unlock(&call_mutex);
		return -ENODEV;
	}

And the password data can simply be directly read from
dell_wmi_bios_attr_data without needing a helper for it at all.

>>>> +
>>>> +     /* password is set */
>>>> +     if (strlen(current_password) > 0)
>>>> +             security_area_size = (sizeof(u32) * 2) +
>> strlen(current_password) +
>>>> +                                     strlen(current_password) % 2;
>>>> +     /* password not set */
>>>> +     else
>>>> +             security_area_size = sizeof(u32) * 2;
>>>
>>> Since you are using more then 1 line here please use {} around the state-
>> ments,
>>> also please put the /* password not set */ after the else:
>>>
>>>                  ...
>>>          } else { /* password not set */
>>>                  ...
>>>
>>>> +     string_area_size = (strlen(a_name) + strlen(a_value))*2;
>>>> +     buffer_size = security_area_size + string_area_size + sizeof(u16) *
>> 2;
>>>> +
>>>> +     buffer = kzalloc(buffer_size, GFP_KERNEL);
>>
>> Actually above looks like home grown kasprintf() implementation.
> 
> I don't think so, sprintf isn't used at all here.  It's a calculation to determine
> the size of the buffer to use.

Ack, this is different because it concats a UTF16 string together with some
other data into the new buf.

Regards,

Hans