linux-kernel - Re: [PATCH] parisc: More csum_ipv6

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]

Date: Tue, 27 Feb 2024 21:14:26 +0100
From: Helge Deller <deller@....de>
To: Guenter Roeck <linux@...ck-us.net>
Cc: "James E . J . Bottomley" <James.Bottomley@...senPartnership.com>,
 Charlie Jenkins <charlie@...osinc.com>, linux-parisc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] parisc: More csum_ipv6_magic fixes

On 2/27/24 19:53, Guenter Roeck wrote:
> IPv6 checksum tests with unaligned addresses resulted in unexpected
> failures.
>
> Expected expected == csum_result, but
>      expected == 46591 (0xb5ff)
>      csum_result == 46381 (0xb52d)
> with alignment offset 1
>
> Oddly enough, the problem disappeared after adding test code into
> the beginning of csum_ipv6_magic().
>
> As it turns out, the 'sum' parameter of csum_ipv6_magic() is declared as
> __wsum, which is a 32-bit variable type. However, it is treated as 64-bit
> variable in the assembler code.

Nice catch!
That kind of bugs is actually the reason why I start to prefer
C-code over inline assembly, even if C might perform slower.

I've applied that patch to the parisc git tree, but do you think
you can come up with a better patch title, e.g.
"strip upper 32bits of sum in csum_ipv6_magic()" ?

Other than that you may add
Acked-by: Helge Deller <deller@....de>

Helge

> Tests showed that the upper 32 bit of
> the register used to pass the variable are _not_ cleared when entering
> the function. This can result in checksum calculation errors.
>
> Clearing the upper 32 bit of 'sum' as first operation in the assembler
> code fixes the problem.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> Maybe there is a way to do this without additional instruction, but if so
> I have not been able to find it.
>
>   arch/parisc/include/asm/checksum.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/parisc/include/asm/checksum.h b/arch/parisc/include/asm/checksum.h
> index c949aa20fa16..2aceebcd695c 100644
> --- a/arch/parisc/include/asm/checksum.h
> +++ b/arch/parisc/include/asm/checksum.h
> @@ -126,6 +126,7 @@ static __inline__ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
>   	** Try to keep 4 registers with "live" values ahead of the ALU.
>   	*/
>
> +"	depdi		0, 31, 32, %0\n"/* clear upper half of incoming checksum */
>   "	ldd,ma		8(%1), %4\n"	/* get 1st saddr word */
>   "	ldd,ma		8(%2), %5\n"	/* get 1st daddr word */
>   "	add		%4, %0, %0\n"