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 for Android: free password hash cracker in your pocket

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

Message-ID: <20191021163208.5cd03d59@endymion>
Date:   Mon, 21 Oct 2019 16:32:08 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     "Erwan Velu" <erwanaliasr1@...il.com>
Cc:     "Robert P. J. Day" <rpjday@...shcourse.ca>,
        "Erwan Velu" <e.velu@...teo.com>,
        "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-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save
 releases fields

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)
>  {


-- 
Jean Delvare
SUSE L3 Support