[PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Andy Shevchenko]

The byte at offset 6 represent length. Don't take it and drop it immediately
by using proper accessor, i.e. get_unaligned_be24().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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);
-- 
2.35.1

Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Jean Delvare]

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

Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Andy Shevchenko]

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

Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Jean Delvare]

Hi Andy,

On Fri, 29 Jul 2022 21:23:08 +0300, Andy Shevchenko wrote:
> 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.

I didn't consider that. Having code match the documentation is indeed
valuable.

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

True.

> > I'm not too sure what is preferred in such situations.
> 
> For cold paths I think the correctness prevail the performance.

You have a point.

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

I considered that, but that's too hackish for me and could easily cause
confusion leading to improper use. Not worth it. Let's keep things
simple and understandable.

So I'll apply your patch, thanks for contributing it.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Andy Shevchenko]

On Sat, Jul 30, 2022 at 11:33:02AM +0200, Jean Delvare wrote:
> On Fri, 29 Jul 2022 21:23:08 +0300, Andy Shevchenko wrote:

...

> So I'll apply your patch, thanks for contributing it.

Thanks for a nice discussion, I have learnt something new!

-- 
With Best Regards,
Andy Shevchenko