Re: [PATCH net 10/14] can: dev: can_put_echo_skb(): free skb on invalid echo index

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 27.08.26 14:10, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Modifying SKB payload in can_skb_init_valid() without unsharing causes data corruption for concurrent observers.
> --
> 
> commit 6a48fe2d96b6f1ca706da34b2f7bdd872d06941f
> Author: Cunhao Lu <1579567540@qq.com>
> can: dev: can_put_echo_skb(): free skb on invalid echo index
> 
> This patch fixes a potential leak in can_put_echo_skb() when the echo index
> is out of bounds. It ensures the socket buffer is freed before returning
> -EINVAL so that all error paths correctly consume the SKB.
> 
> Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com
> 
>> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
>> index c7be21ed70ddf..faafb1033d423 100644
>> --- a/drivers/net/can/dev/skb.c
>> +++ b/drivers/net/can/dev/skb.c
> 
> [ ... ]
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but already existed in the
> codebase. Does can_skb_init_valid() in this same file modify shared memory
> without unsharing it first?
> 
> drivers/net/can/dev/skb.c:can_skb_init_valid() {
>          ...
>          /* set CANFD_FDF flag for CAN FD frames */
>          if (can_is_canfd_skb(skb)) {
>              struct canfd_frame *cfd;
> 
>              cfd = (struct canfd_frame *)skb->data;
>              cfd->flags |= CANFD_FDF;
>          }
>          ...
> }
> 
> If a user with CAP_NET_RAW injects a CAN FD frame via an AF_PACKET socket,
> the packet goes to dev_queue_xmit() where it may be cloned for packet
> sniffers like tcpdump.
> 
> When the original SKB reaches the driver's transmit function and calls
> can_dev_dropped_skb() -> can_dropped_invalid_skb() -> can_skb_init_valid(),
> it modifies the payload (cfd->flags |= CANFD_FDF) without verifying
> skb_cloned(skb) or calling skb_unshare().
> 
> Can this cause data corruption for concurrent observers receiving the clones?
> 

No.

https://lore.kernel.org/linux-can/ac22b59e-129b-4883-b9c6-da5e69e4078d@hartkopp.net/