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: <YuQljCM4LZXhSkbh@smile.fi.intel.com>
Date:   Fri, 29 Jul 2022 21:23:08 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jean Delvare <jdelvare@...e.de>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into
 consideration in dmi_smbios3_present()

On Wed, Jul 27, 2022 at 10:25:04AM +0200, Jean Delvare wrote:
> 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.

I don't know how to improve it, but it kinda a bug in a logic for non-prepared
reader, esp. as specification suggests different thing about version offset.

...

> > -		dmi_ver = get_unaligned_be32(buf + 6) & 0xFFFFFF;
> > +		dmi_ver = get_unaligned_be24(buf + 7);

> 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.

Good to know!
But here it's not a hot path, right?

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

For cold paths I think the correctness prevail the performance.

Alternatively we might add a comment in the code explaining the trick,
although I won't like to do it.

Another way is to have a subset of 24-/48-bit unaligned accessors that
use the trick specifically for hot paths with a caveat that they may
access data out of the 24-/48-bit boundaries.

-- 
With Best Regards,
Andy Shevchenko