linux-kernel - Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save

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: <c1dce47f-f772-b6a9-745f-49a39788b276@criteo.com>
Date:   Wed, 27 Nov 2019 15:04:34 +0000
From:   Erwan Velu <e.velu@...teo.com>
To:     Jean Delvare <jdelvare@...e.de>,
        Erwan Velu <erwanaliasr1@...il.com>
CC:     "Robert P. J. Day" <rpjday@...shcourse.ca>,
        Changbin Du <changbin.du@...el.com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Jens Wiklander <jens.wiklander@...aro.org>,
        Sumit Garg <sumit.garg@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michal Marek <michal.lkml@...kovi.net>,
        Mattias Jacobsson <2pi@....nu>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        "linux-kbuild@...r.kernel.org" <linux-kbuild@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save
 releases fields

Got all your points, sending a V2 with the fixes you asked.

On 21/10/2019 16:32, Jean Delvare wrote:
> Hi Erwan,
>
> Sorry for the late answer.
>
> On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote:
>> In DMI type 0, there is several fields that encodes a release.
> is -> are
> encodes -> encode
>
>> The dmi_save_release() function have the logic to check if the field is valid.
>> If so, it reports the actual value.
>>
>> Signed-off-by: Erwan Velu <e.velu@...teo.com>
>> ---
>>   drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
> This patch introduces a warning:
>
> drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not used [-Wunused-function]
>   static void __init dmi_save_release(const struct dmi_header *dm, int slot,
>                      ^~~~~~~~~~~~~~~~
>
> because you add a static function with no user. I understand that you
> add a use later in the series, but it's not OK to introduce a warning
> even if temporary. It also makes little sense to split the changes that
> way as there is no way to cherry-pick one of the patches without the
> rest. And it makes things more difficult to review too, as I can't
> possibly judge if this function if right without also seeing where is
> will be called and how.
>
> So, please merge all the changes into a single patch.
>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index 35ed56b9c34f..202bd2c69d0f 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot,
>>   	dmi_ident[slot] = p;
>>   }
>>   
>> +static void __init dmi_save_release(const struct dmi_header *dm, int slot,
>> +		int index)
>> +{
>> +	const u8 *d;
>> +	char *s;
>> +
>> +	// If the table doesn't have the field, let's return
> Please stick to C89-style comments (/* */) as used everywhere else in
> this file.
>
>> +	if (dmi_ident[slot] || dm->length < index)
>> +		return;
>> +
>> +	d = (u8 *) dm + index;
>> +
>> +	// As per the specification,
>> +	// if the system doesn't have the field, the value is FF
>> +	if (d[0] == 0xFF)
>> +		return;
> That's not exactly what the specification says. It says:
>
> "If the system does not support the use of [the System BIOS Major
> Release] field, the value is 0FFh for both this field and the System
> BIOS Minor Release field." So unused is when both fields are 0xFF. You
> can't test them independently.
>
> Same goes for the Embedded Controller Firmware Release fields, even if
> it is worded differently, the logic is the same.
>
>> +
>> +	s = dmi_alloc(4);
>> +	if (!s)
>> +		return;
>> +
>> +	sprintf(s, "%u", d[0]);
>> +
>> +	dmi_ident[slot] = s;
>> +}
>> +
>>   static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
>>   		int index)
>>   {
>