linux-kernel - Re: [PATCH] Staging: vt6655: Fix sparse warning. Restricted cast.

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>] [day] [month] [year] [list]

Message-ID: <b62deb16-daa0-412e-8e11-bf45b88d3107@moroto.mountain>
Date: Wed, 10 Jan 2024 10:25:30 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Ariel Silver <arielsilver77@...il.com>
Cc: forest@...ttletooquiet.net, gregkh@...uxfoundation.org,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: vt6655: Fix sparse warning. Restricted cast.

Hm...  This stuff is more broken than I realized when I first looked at
it.  And the truth is that I don't know the hardware so I can't say
exactly what the fix is.  Probably best to just leave it alone for now...

On Wed, Jan 10, 2024 at 08:25:33AM +0200, Ariel Silver wrote:
> >> Hello Dan and thank you for the quick response. Much appreciated!
> >> 1) I am probably wrong, but as I see it, the current code assumes that
> >> 'CARDqGetTSFOffset' returns a little endian value.
> >> So it calls 'qwTSFOffset =  le64_to_cpu(qwTSFOffset)'. However digging in
> >> the code of 'CARDqGetTSFOffset' I don't see a reason to assume that.
> >> Which in my opinion can cuase a bad endianness cast on big endianness
> >> systems.

You're actually correct.

But the thing is that le64_to_cpu() and cpu_to_le64() do the exactly the
same thing.  If the hardware is little endian they do nothing, but if
the hardware is big endian they call bswap_64().  The only difference is
how a human reader understands it and for git annotations.

What I suspect is that we should be passing little endian data to
iowrite32().  In other words we should change the le64_to_cpu() to
cpu_to_le64().  This wouldn't change how the code works, it would just
change how it is annotated.  However, on the other hand, I see plenty of
examples where it's passing CPU endian data to iowrite32() so maybe we
should get rid of the conversion instead.  Who knows?

I suspect is that this driver has never worked on big endian CPUs...

I would say let's not change this until we're more sure what's going on.
And probably if we make changes that affect runtime as opposed to just
modifying endian annotations, then that needs to be tested.

regards,
dan carpenter