Message-ID: <aSx++4VrGOm8zHDb@inspiron>
Date: Sun, 30 Nov 2025 22:59:31 +0530
From: Prithvi Tambewagh <activprithvi@...il.com>
To: Oliver Hartkopp <socketcan@...tkopp.net>, mkl@...gutronix.de
Cc: linux-can@...r.kernel.org, linux-kernel@...r.kernel.org,
	syzkaller-bugs@...glegroups.com, netdev@...r.kernel.org
Subject: Re: Question about to KMSAN: uninit-value in can_receive

On Sun, Nov 30, 2025 at 01:44:32PM +0100, Oliver Hartkopp wrote:
>
>
>On 29.11.25 18:04, Oliver Hartkopp wrote:
>>Hello Prithvi,
>>
>>thanks for picking up this topic!
>>
>>I had your mail in my open tabs and I was reading some code several 
>>times without having a really good idea how to continue.
>>
>>On 17.11.25 18:30, Prithvi Tambewagh wrote:
>>
>>>The call trace suggests that the bug appears to be due to effect 
>>>of change
>>>in headroom by pskb_header_expand(). The new headroom remains 
>>>uninitialized
>>>and when can_receive tries accessing can_skb_prv(skb)->skbcnt, indirectly
>>>skb->head is accessed which causes KMSAN uninitialized value read bug.
>>
>>Yes.
>>
>>If you take a look at the KMSAN message:
>>
>>https://lore.kernel.org/linux- 
>>can/68bae75b.050a0220.192772.0190.GAE@...gle.com/T/ 
>>#m0372e223746b9da19cbf39348ab1cda52a5cfadc
>>
>>I wonder why anybody is obviously fiddling with the with the 
>>skb->head here.
>>
>>When initially creating skb for the CAN subsystem we use 
>>can_skb_reserve() which does a
>>
>>skb_reserve(skb, sizeof(struct can_skb_priv));
>>
>>so that we get some headroom for struct can_skb_priv.
>>
>>Then we access this struct by referencing skb->head:
>>
>>static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>>{
>>     return (struct can_skb_priv *)(skb->head);
>>}
>>
>>If anybody is now extending the headroom skb->head will likely not 
>>pointing to struct can_skb_priv anymore, right?
>>
>>>To fix this bug, I think we can call can_dropped_invalid_skb() in 
>>>can_rcv()
>>>just before calling can_receive(). Further, we can add a condition 
>>>for these
>>>sk_buff with uninitialized headroom to initialize the skb, the way it had
>>>been done in the patch for an earlier packet injection case in a similar
>>>KMSAN bug:
>>>https://lore.kernel.org/linux-can/20191207183418.28868-1- 
>>>socketcan@...tkopp.net/
>>
>>No. This is definitely a wrong approach. You can not wildly poke 
>>values behind skb->head, when the correctly initialized struct 
>>can_skb_priv just sits somewhere else.
>>
>>In opposite to the case in your referenced patch we do not get a skb 
>>from PF_PACKET but we handle a skb that has been properly created in 
>>isotp_sendmsg(). Including can_skb_reserve() and an initialized 
>>struct can_skb_priv.
>>
>>>However, I am not getting on what basis can I filter the sk_buff so that
>>>only those with an uninitialized headroom will be initialized via 
>>>this path.
>>>Is this the correct approach?
>>
>>No.
>>
>>When we are creating CAN skbs with [can_]skb_reserve(), the struct 
>>can_skb_priv is located directly "before" the struct can_frame which 
>>is at skb->data.
>>
>>I'm therefore currently thinking in the direction of using skb->data 
>>instead of skb->head as reference to struct can_skb_priv:
>>
>>diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>>index 1abc25a8d144..8822d7d2e3df 100644
>>--- a/include/linux/can/skb.h
>>+++ b/include/linux/can/skb.h
>>@@ -60,11 +60,11 @@ struct can_skb_priv {
>>         struct can_frame cf[];
>>  };
>>
>>  static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
>>  {
>>-       return (struct can_skb_priv *)(skb->head);
>>+       return (struct can_skb_priv *)(skb->data - sizeof(struct 
>>can_skb_priv));
>>  }
>>
>>  static inline void can_skb_reserve(struct sk_buff *skb)
>>  {
>>         skb_reserve(skb, sizeof(struct can_skb_priv));
>>
>>I have not checked what effect this might have to this patch
>>
>>https://lore.kernel.org/linux-can/20191207183418.28868-1- 
>>socketcan@...tkopp.net/
>>
>>when we initialize struct can_skb_priv inside skbs we did not create 
>>in the CAN subsystem. The difference would be that we access struct 
>>can_skb_priv via skb->data and not via skb->head. The effect to the 
>>system should be similar.
>>
>>What do you think about such approach?
>>
>>Best regards,
>>Oliver
>>
>
>Hello Prithvi,
>
>I'm answering in this mail thread as you answered on the other thread 
>which does not preserve the discussion above.

Hello Oliver,

Apologies for this, I was using git send-email and probably messed up with
the Message ID. I have just set up mutt, this should be correct now.

>
>On 30.11.25 13:04, Prithvi Tambewagh wrote:
>> Hello Oliver,
>>
>> Thanks for the feedback! I now understand how struct can_skb_priv is
>> reserved in the headroom, more clearly, given that I am relatively new
>> to kernel development. I agree on your patch.
>>
>> I tested it locally  using the reproducer program for this bug 
>provided by
>> syzbot and it didn't crash the kernel. Also, I checked the patch here
>>
>> https://lore.kernel.org/linux-can/20191207183418.28868-1-socketcan@hartkopp.net/
>>
>> looking at it, I think your patch will work fine with the above patch as
>> well, since data will be accessed at
>>
>> skb->data - sizeof(struct can_skb_priv)
>>
>> which is the intended place for it, according to te action of
>> can_skb_reserve() which increases headroom by length
>> sizeof(struct can_skb_priv), reserving the space just before skb->data.
>>
>> I think it solves this specific KMSAN bug. Kindly correct me if I am 
>wrong.
>
>Yes. It solves that specific bug. But IMO we need to fix the root 
>cause of this issue.
>
>The CAN skb is passed to NAPI and XDP code
>
> kmalloc_reserve+0x23e/0x4a0 net/core/skbuff.c:609
> pskb_expand_head+0x226/0x1a60 net/core/skbuff.c:2275
> netif_skb_check_for_xdp net/core/dev.c:5081 [inline]
> netif_receive_generic_xdp net/core/dev.c:5112 [inline]
> do_xdp_generic+0x9e3/0x15a0 net/core/dev.c:5180
> __netif_receive_skb_core+0x25c3/0x6f10 net/core/dev.c:5524
>
>which invoked pskb_expand_head() which manipulates skb->head and 
>therefore removes the reference to our struct can_skb_priv.
>> Would you like to fix this bug by sending your patch upstream? Or else
>> shall I send this patch upstream and mention your name in 
>Suggested-by tag?
>
>No. Neither of that - as it will not fix the root cause.
>
>IMO we need to check who is using the headroom in CAN skbs and for 
>what reason first. And when we are not able to safely control the 
>headroom for our struct can_skb_priv content we might need to find 
>another way to store that content.
>E.g. by creating this space behind skb->data or add new attributes to 
>struct sk_buff.

I will work in this direction. Just to confirm, what you mean is
that first it should be checked where the headroom is used while also
checking whether the data from region covered by struct can_skb_priv is 
intact, and if not then we need to ensure that it is intact by other 
measures, right? 

>
>Best regards,
>Oliver

Thank You,
Prithvi

Powered by blists - more mailing lists