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

[Sabrina Dubroca <sd@queasysnail.net>]

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