Re: Question about to KMSAN: uninit-value in can_receive

[Oliver Hartkopp]

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@xxxxxxxxxx/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@xxxxxxxxxxxx/

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@xxxxxxxxxxxx/

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