[GIT PULL] dmi update for v5.19

[GIT PULL] dmi update for v5.19

[Jean Delvare]

Hi Linus,

Please pull 1 dmi subsystem update for Linux v5.19 from:

git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-linus


 drivers/firmware/dmi_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

---------------

Andy Shevchenko (1):
      firmware: dmi: Use the proper accessor for the version field

Sorry for requesting a pull outside of the merge window, but I was on
vacation last 2 weeks.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

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

[Jean Delvare]

On Mon, 22 Aug 2022 14:19:30 +0200, Jean Delvare wrote:
> Please pull 1 dmi subsystem update for Linux v5.19 from:

s/v5\.19/v6.0/, of course.

-- 
Jean Delvare
SUSE L3 Support

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

[Linus Torvalds]

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.

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?

           Linus

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

[Andy Shevchenko]

On Wed, Aug 24, 2022 at 8:41 PM Linus Torvalds
<torvalds@linux-foundation.org> 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.

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

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

[pr-tracker-bot]

The pull request you sent on Mon, 22 Aug 2022 14:19:30 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging.git dmi-for-linus

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e78bf8cbf005c3cc7dc4da55ce75152b71a1da0f

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Jean Delvare]

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