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) */
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.
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.
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
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.
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
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.
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.
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.
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.
linux-stable-mirror@lists.linaro.org
-
Jacob Keller -
Jakub Kicinski -
Nate Karstens -
Sabrina Dubroca