Message-ID: <c2cead0a-06ed-4da4-a4e4-8498908aae3e@hartkopp.net>
Date: Sun, 30 Nov 2025 13:44:32 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Prithvi Tambewagh <activprithvi@...il.com>, 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 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.

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.

Best regards,
Oliver

Powered by blists - more mailing lists