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

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?

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

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?

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