[PATCH] strparser: Fix signed/unsigned mismatch bug

[PATCH] strparser: Fix signed/unsigned mismatch bug

[Nate Karstens]

The `len` member of the sk_buff is an unsigned int. This is cast to
`ssize_t` (a signed type) for the first sk_buff in the comparison,
but not the second sk_buff. This change ensures both len values are
cast to `ssize_t`.

This appears to cause an issue with ktls when multiple TLS PDUs are
included in a single TCP segment.

Signed-off-by: Nate Karstens <nate.karstens@garmin.com>
Cc: stable@vger.kernel.org
---
 net/strparser/strparser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 43b1f558b33d..e659fea2da70 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -238,7 +238,7 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 				strp_parser_err(strp, -EMSGSIZE, desc);
 				break;
 			} else if (len <= (ssize_t)head->len -
-					  skb->len - stm->strp.offset) {
+					  (ssize_t)skb->len - stm->strp.offset) {
 				/* Length must be into new skb (and also
 				 * greater than zero)
 				 */
-- 
2.34.1

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

[Sabrina Dubroca]

2025-11-04, 11:42:03 -0600, Nate Karstens wrote:
> The `len` member of the sk_buff is an unsigned int. This is cast to
> `ssize_t` (a signed type) for the first sk_buff in the comparison,
> but not the second sk_buff. This change ensures both len values are
> cast to `ssize_t`.
> 
> This appears to cause an issue with ktls when multiple TLS PDUs are
> included in a single TCP segment.

Can you describe a bit more the problematic case (state of the
strparser and all the variables involved maybe?), and how the added
cast fixes it?

And what kernel version are you using to trigger this issue (and then
verify the fix)? ktls hasn't used net/strparser for quite a while (see
commit 84c61fe1a75b ("tls: rx: do not use the standard strparser")).

> Signed-off-by: Nate Karstens <nate.karstens@garmin.com>

A Fixes: tag would also be good, and the subject prefix should be
"[PATCH net]" for bugfixes.

-- 
Sabrina

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

[Nate Karstens]

All right, one more time using `git send-email` (plainly I don't do this every day)...

Sabrina,

Thanks for looking at this!

I'm seeing this on kernel version 5.10.244. I know that ktls on the mainline kernel has moved away from strparser, but I think the change would be useful for anyone still using strparser (both for ktls on old kernels and other users as well). It seems that, because head->len was cast to ssize_t, it was an oversight that skb->len wasn't as well (if the intention was to use unsigned arithmetic, then there would be no need to cast head-> len).

Here is an example of the values involved with the test I'm running:

len = 16406
head->len = 1448
skb->len = 1448
stm->strp.offset = 478
(ssize_t)head->len - skb->len - stm.strp.offset = 4294966818
(ssize_t)head->len - (ssize_t)skb->len - stm.strp.offset = -478

I'm happy to update the patch, how much of this information would be useful to include in the commit message?

Nate

________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

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

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1677 bytes --]



On 11/5/2025 9:34 AM, Nate Karstens wrote:
> All right, one more time using `git send-email` (plainly I don't do this every day)...
> 
> Sabrina,
> 
> Thanks for looking at this!
> 
> I'm seeing this on kernel version 5.10.244. I know that ktls on the mainline kernel has moved away from strparser, but I think the change would be useful for anyone still using strparser (both for ktls on old kernels and other users as well). It seems that, because head->len was cast to ssize_t, it was an oversight that skb->len wasn't as well (if the intention was to use unsigned arithmetic, then there would be no need to cast head-> len).
>

Right.
> Here is an example of the values involved with the test I'm running:
> 
> len = 16406
> head->len = 1448
> skb->len = 1448
> stm->strp.offset = 478
> (ssize_t)head->len - skb->len - stm.strp.offset = 4294966818

So, without the ssize_t, I guess everything switches back to unsigned
here when subtracting skb->len..

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?
> (ssize_t)head->len - (ssize_t)skb->len - stm.strp.offset = -478

Where as here, it resolves to signed 0, so we go ultimately resolve to a
signed result?

> 
> I'm happy to update the patch, how much of this information would be useful to include in the commit message?
> 

If we don't actually use the strparser code anywhere then it could be
dropped? But otherwise I agree with Nate that we shouldn't leave this
mistake in place, even if its not actually used by kTLS anymore.

Thanks,
Jake


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Nate Karstens]

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.

> 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)

> 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.

Cheers,

Nate

________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

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

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1511 bytes --]



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.
> 
>> 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.
>> 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.

> Cheers,
> 
> Nate

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Sabrina Dubroca]

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

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

[Nate Karstens]

Thanks, Sabrina!

> 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?

Yes, that is a good point. I tested this on a 32-bit architecture. On a 64-bit system, the u32 would be put into an s64 because all possible values for the u32 can fit into the s64. Signed arithmetic is used and you would get the correct result.

> Agree. And adding a summary of the information in this thread to the
> commit message would be really useful

Sounds good!

> 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.

No worries, I didn't take it as being dismissive at all. You had great questions and I agree that the discussion has been really helpful!

Cheers,

Nate

________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

[PATCH net v2] strparser: Fix signed/unsigned mismatch bug

[Nate Karstens]

The `len` member of the sk_buff is an unsigned int. This is cast to
`ssize_t` (a signed type) for the first sk_buff in the comparison,
but not the second sk_buff. On 32-bit systems, this can result in
an integer underflow for certain values because unsigned arithmetic
is being used.

This appears to be an oversight: if the intention was to use unsigned
arithmetic, then the first cast would have been omitted. The change
ensures both len values are cast to `ssize_t`.

The underflow causes an issue with ktls when multiple TLS PDUs are
included in a single TCP segment. The mainline kernel does not use
strparser for ktls anymore, but this is still useful for other
features that still use strparser, and for backporting.

Signed-off-by: Nate Karstens <nate.karstens@garmin.com>
Cc: stable@vger.kernel.org
Fixes: 43a0c6751a32 ("strparser: Stream parser for messages")
---
 net/strparser/strparser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 43b1f558b33d..e659fea2da70 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -238,7 +238,7 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
                                strp_parser_err(strp, -EMSGSIZE, desc);
                                break;
                        } else if (len <= (ssize_t)head->len -
-                                         skb->len - stm->strp.offset) {
+                                         (ssize_t)skb->len - stm->strp.offset) {
                                /* Length must be into new skb (and also
                                 * greater than zero)
                                 */
--
2.34.1


________________________________

CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.

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

[Jakub Kicinski]

On Thu, 6 Nov 2025 10:51:17 -0600 Nate Karstens wrote:
> CONFIDENTIALITY NOTICE: This email and any attachments are for the
> sole use of the intended recipient(s) and contain information that
> may be Garmin confidential and/or Garmin legally privileged. If you
> have received this email in error, please notify the sender by reply
> email and delete the message. Any disclosure, copying, distribution
> or use of this communication (including attachments) by someone other
> than the intended recipient is prohibited. Thank you.

This notice prevents us from doing anything with the patch.

Also please do _not_ send the patches in reply to existing threads.