linux-kernel - Re: [PATCH] strparser: Fix signed/unsigned mismatch bug

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: <aQy9TZm4rpP3ZB4b@krikkit>
Date: Thu, 6 Nov 2025 16:22:53 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Nate Karstens <nate.karstens@...min.com>, davem@...emloft.net,
	edumazet@...gle.com, horms@...nel.org, john.fastabend@...il.com,
	kuba@...nel.org, linux-kernel@...r.kernel.org, linux@...blig.org,
	mrpre@....com, nate.karstens@...il.com, netdev@...r.kernel.org,
	pabeni@...hat.com, stable@...r.kernel.org, tom@...ntonium.net
Subject: Re: [PATCH] strparser: Fix signed/unsigned mismatch bug

2025-11-05, 15:47:00 -0800, Jacob Keller wrote:
> 
> 
> On 11/5/2025 3:12 PM, Nate Karstens wrote:
> > Thanks, Jake!
> > 
> >> So, without the ssize_t, I guess everything switches back to unsigned
> >> here when subtracting skb->len..
> > 
> > That's right. In C, if there is a mix of signed an unsigned, then signed are converted to unsigned and unsigned arithmetic is used.

Not if the signed type is bigger than the unsigned?

on x86_64 (with long = s64 and unsigned int = u32):
(long)1 - (unsigned int)100  <  0
(int)1  - (unsigned int)100  >  0

Are you testing on some 32b arch? Otherwise ssize_t would be s64 and
int/unsigned int should be 32b so the missing cast would not matter?


> >> I don't quite recall the signed vs unsigned rules for this. Is
> >> stm.strp.offset also unsigned? which means that after head->len -
> >> skb->len resolves to unsigned 0 then we underflow?
> > 
> > Here is a summary of the types for the variables involved:
> > 
> > len => ssize_t (signed)
> > (ssize_t)head->len => unsigned int cast to ssize_t
> > skb->len => unsigned int, causes the whole comparison to use unsigned arithmetic
> > stm->strp.offset => int (see struct strp_msg)
> > 
> 
> Ah, right so if we don't cast skb->len then the entire thing uses
> unsigned arithmetic which results in the bad outcome for certain values
> of input.
> 
> Casting would fix this. Another alternative would be to re-write the
> checks so that they don't fail when using unsigned arithmetic.
> 
> Given that we already cast one to ssize_t, it does seem reasonable to
> just add the other cast as your patch did.

Agree. And adding a summary of the information in this thread to the
commit message would be really useful (clearly, this stuff is not so
obvious :)).

> >> If we don't actually use the strparser code anywhere then it could be
> >> dropped
> > 
> > It is still used elsewhere, and ktls still uses some of the data structures.
> > 
> 
> Right. Fixing it makes the most sense, so that other users don't
> accidentally behave unexpectedly.

Agree. I didn't mean to dismiss the presence of a bug, sorry if it
sounded like that. But I was a bit unclear on the conditions, this
discussion is helpful.

-- 
Sabrina