[PATCH] can: dev: can_put_echo_skb(): free skb on invalid echo index

[PATCH] can: dev: can_put_echo_skb(): free skb on invalid echo index

[Cunhao Lu]

can_put_echo_skb() consumes the skb on all paths except when the echo index
is out of bounds. This leaves ownership with the caller on -EINVAL, unlike
the other error paths, and can leak the skb if the caller expects
consistent semantics.

Free the skb before returning -EINVAL so that all return paths consume it.

Fixes: 6411959c10fe ("can: dev: can_put_echo_skb(): don't crash kernel if can_priv::echo_skb is accessed out of bounds")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>
---
 drivers/net/can/dev/skb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..44ebeba99837 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
 			   __func__, idx, priv->echo_skb_max);
+		kfree_skb(skb);
 		return -EINVAL;
 	}
 

---
base-commit: 11028ab62899e4191e074ee364c712b77823a9c4
change-id: 20260730-master-abf3e4067e47

Best regards,
--  
Cunhao Lu <1579567540@qq.com>

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Context-unsafe SKB free in can_put_echo_skb()
--

--- Patch [1]: [PATCH] can: dev: can_put_echo_skb(): free skb on invalid echo index ---
commit ab12134887377b0b69b9ab5b16cf5eb0ddf3eb9e
Author: Cunhao Lu <1579567540@qq.com>

can: dev: can_put_echo_skb(): free skb on invalid echo index

This commit fixes a memory leak in can_put_echo_skb() by ensuring the skb is
freed when the echo index is out of bounds. It adds a kfree_skb() call to
the -EINVAL error path.

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f80..44ebeba998370 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	if (idx >= priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
>  			   __func__, idx, priv->echo_skb_max);
> +		kfree_skb(skb);

[Severity: High]
This is a pre-existing issue in the surrounding code, but does adding another
kfree_skb() here introduce an unsafe free context?

Since can_put_echo_skb() is commonly called from the ndo_start_xmit path of
CAN drivers while holding an irqsave spinlock (such as in
peak_canfd_start_xmit()), hardware interrupts may be disabled.

Calling kfree_skb() with interrupts disabled can be unsafe because SKB
destructors like sock_wfree() might assume they are in process or softirq
context, potentially leading to lockups or warnings.

Would it be safer to use dev_kfree_skb_any() here, and possibly update the
other pre-existing kfree_skb() and consume_skb() calls in this function as
well?

>  		return -EINVAL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_944DADCC4B42C8484EC01DA2B15F42132906@qq.com?part=1

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

[Vincent Mailhol]

On Thu. 30 juil. 2026 at 17:03, Cunhao Lu <1579567540@qq.com> wrote:
>
> can_put_echo_skb() consumes the skb on all paths except when the echo index
> is out of bounds. This leaves ownership with the caller on -EINVAL, unlike
> the other error paths, and can leak the skb if the caller expects
> consistent semantics.
>
> Free the skb before returning -EINVAL so that all return paths consume it.
>
> Fixes: 6411959c10fe ("can: dev: can_put_echo_skb(): don't crash kernel if can_priv::echo_skb is accessed out of bounds")
> Cc: stable@vger.kernel.org

This branch is just a safety net meant to never be reached. While the
fix logically makes sense, this doesn't strictly require a backport.
That said, I am not against this being backported if the stable team
is fine with it.

> Signed-off-by: Cunhao Lu <1579567540@qq.com>

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>

> ---
>  drivers/net/can/dev/skb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 95fcdc1026f8..44ebeba99837 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -54,6 +54,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>         if (idx >= priv->echo_skb_max) {
>                 netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
>                            __func__, idx, priv->echo_skb_max);
> +               kfree_skb(skb);
>                 return -EINVAL;
>         }

Yours sincerely,
Vincent Mailhol