linux-kernel - Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3

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: <20220727102504.6bbefcf9@endymion.delvare>
Date:   Wed, 27 Jul 2022 10:25:04 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into
 consideration in dmi_smbios3_present()

Hi Andy,

On Tue, 26 Jul 2022 12:43:29 +0300, Andy Shevchenko wrote:
> The byte at offset 6 represent length. Don't take it and drop it immediately
> by using proper accessor, i.e. get_unaligned_be24().

The subject sounds like you are fixing a bug, while this is only, at
best, a minor optimization.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index b2ea318a10a4..24537ce29bc4 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -630,7 +630,7 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  {
>  	if (memcmp(buf, "_SM3_", 5) == 0 &&
>  	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
> -		dmi_ver = get_unaligned_be32(buf + 6) & 0xFFFFFF;
> +		dmi_ver = get_unaligned_be24(buf + 7);
>  		dmi_num = 0;			/* No longer specified */
>  		dmi_len = get_unaligned_le32(buf + 12);
>  		dmi_base = get_unaligned_le64(buf + 16);

I admit I did not know about get_unaligned_be24(). While I agree that
it makes the source code look better, one downside is that it actually
increases the binary size on x86_64. The reason is that
get_unaligned_be32() is optimized by assembly instruction bswapl, while
get_unaligned_be24() is not. Situation appears to be the same on ia64
and arm. Only arm64 would apparently benefit from your proposed
change.

I'm not too sure what is preferred in such situations.

-- 
Jean Delvare
SUSE L3 Support