Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Sep 2022 09:56:54 +0200
From:   Jean Delvare <jdelvare@suse.de>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [GIT PULL] dmi update for v5.19
Message-ID: <20220907095654.4b1ea5b0@endymion.delvare>
In-Reply-To: <CAHk-=wiFRok=hU_BNEsqodjyGm=XV9LVZ1w=cm4ooEtWVpYLPg@mail.gmail.com>
References: <20220822141930.5f43b5e7@endymion.delvare>
        <CAHk-=wiFRok=hU_BNEsqodjyGm=XV9LVZ1w=cm4ooEtWVpYLPg@mail.gmail.com>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Linus,

On Wed, 24 Aug 2022 10:31:35 -0700, Linus Torvalds wrote:
> On Mon, Aug 22, 2022 at 5:19 AM Jean Delvare <jdelvare@suse.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.

For reference, I had the same objection originally, but Andy convinced
me that code clarity was more important than a minor one-time
optimization. The whole discussion can be read here:

https://lore.kernel.org/all/YuVUdOUl7zwE0QsV@smile.fi.intel.com/T/#mf41d04beeb1d4fddadf77248eec8be397f77cdb5

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

Thanks for taking the time to look into this. You have a point.

It doesn't utterly matter in practice because it's hard to imagine that
the checksum would be correct if the size is not. The check for
size < 32 is to avoid a walking past the end of the buffer if the entry
point is incorrect or corrupted. Every other case of incorrectness or
corruption is assumed to be caught by the checksum itself.

I suppose that the lack of a check for a minimum size comes from the
fact that the legacy DMI entry point did not even have a size field. As
a matter of fact, dmidecode does not have a minimum entry point size
check either.

I can add a minimum entry size check if you want. Some extra robustness
can't hurt.

Thanks,
-- 
Jean Delvare
SUSE L3 Support