[PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[Frederik Deweerdt]

Hi!

We see `tls_strp_check_queue_ok` running into a NULL deref when
evaluating `TCP_SKB_CB(skb)->seq`.

This commit attempts to address the issue by exiting the loop if
skb->next is NULL, and has proven stable under load.

That said i don't understand the code enough to convince myself that
the NULL check is indeed required, and i would be happy gather data if
that's useful.

Signed-off-by: Frederik Deweerdt <deweerdt.lkml@gmail.com>
---
 net/tls/tls_strp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
index ca1e0e198ceb..cabac0db6445 100644
--- a/net/tls/tls_strp.c
+++ b/net/tls/tls_strp.c
@@ -441,6 +441,8 @@ static bool tls_strp_check_queue_ok(struct tls_strparser *strp)
 		len -= skb->len;
 		skb = skb->next;
 
+		if (!skb)
+			return false;
 		if (TCP_SKB_CB(skb)->seq != seq)
 			return false;
 		if (skb_cmp_decrypted(first, skb))
-- 
2.42.0

Re: [PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[Jakub Kicinski]

On Mon, 30 Oct 2023 14:57:29 -0700 Frederik Deweerdt wrote:
> We see `tls_strp_check_queue_ok` running into a NULL deref when
> evaluating `TCP_SKB_CB(skb)->seq`.
> 
> This commit attempts to address the issue by exiting the loop if
> skb->next is NULL, and has proven stable under load.
> 
> That said i don't understand the code enough to convince myself that
> the NULL check is indeed required, and i would be happy gather data if
> that's useful.

Hm. Can you share the decoded stack trace?

Re: [PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[Frederik Deweerdt]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Mon, Oct 30, 2023 at 03:05:12PM -0700, Jakub Kicinski wrote:
> On Mon, 30 Oct 2023 14:57:29 -0700 Frederik Deweerdt wrote:
> > We see `tls_strp_check_queue_ok` running into a NULL deref when
> > evaluating `TCP_SKB_CB(skb)->seq`.
> > 
> > This commit attempts to address the issue by exiting the loop if
> > skb->next is NULL, and has proven stable under load.
> > 
> > That said i don't understand the code enough to convince myself that
> > the NULL check is indeed required, and i would be happy gather data if
> > that's useful.
> 
> Hm. Can you share the decoded stack trace?

We could only have screen captures from the management console. I've
attached the image to this email.

Frederik

[-- Attachment #2: trace.png --]
[-- Type: image/png, Size: 240647 bytes --]

Re: [PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[Jakub Kicinski]

On Mon, 30 Oct 2023 15:20:05 -0700 Frederik Deweerdt wrote:
> > Hm. Can you share the decoded stack trace?  
> 
> We could only have screen captures from the management console. I've
> attached the image to this email.

Is it possible to enable crash dump kernel or serial console or
netconsole on the machines to catch a fuller stack trace?

Re: [PATCH] [RFC] Add missing NULL check in `tls_strp_check_queue_ok`

[Frederik Deweerdt]

On Mon, Oct 30, 2023 at 04:24:54PM -0700, Jakub Kicinski wrote:
> On Mon, 30 Oct 2023 15:20:05 -0700 Frederik Deweerdt wrote:
> > > Hm. Can you share the decoded stack trace?  
> > 
> > We could only have screen captures from the management console. I've
> > attached the image to this email.
> 
> Is it possible to enable crash dump kernel or serial console or
> netconsole on the machines to catch a fuller stack trace?

It might me, let me ask internally.

Thanks!
Frederik