linux-kernel - Re: [GIT PULL] dmi update for v5.19

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: <CAHp75Vd46=Q65B4L_Lzk+HuzGpPf0RkBvfPfM2UccLQWvwwqpQ@mail.gmail.com>
Date:   Wed, 24 Aug 2022 21:02:26 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jean Delvare <jdelvare@...e.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] dmi update for v5.19

On Wed, Aug 24, 2022 at 8:41 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Mon, Aug 22, 2022 at 5:19 AM Jean Delvare <jdelvare@...e.de> wrote:
> >
> > Andy Shevchenko (1):
> >       firmware: dmi: Use the proper accessor for the version field
>
> I pulled this, but I kind of question it.
>
> This replaces a single 32-bit memory access (and an optimized byte
> swap) and a mask operation with three load-byte-and-shift operations.
>
> It's not clear that the new code is better.

That concern was arisen during discussion, but my point here is that
when you read the SMBus specification and look at the offset 6 the
operation on it, even optimized, may confuse the reader. At least it
confused me. Hence the patch.

> That said, I can't imagine it matters - but because I looked at it, I
> note that the length check seems to be kind of iffy.
>
> The code checks that the length of the block is < 32 before doing the
> checksum on it, but shouldn't it also check for some minimum size?
> Otherwise the dmi checksum is kind of pointless, isn't it?
>
> It will access a minimum of 24 bytes for that dmi_base thing, so that
> would be the most obvious minimum value. But maybe there is some
> spec-defined size for that that only covers the header?

-- 
With Best Regards,
Andy Shevchenko