netdev - Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201103172102.3d75cb96@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 3 Nov 2020 17:21:02 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        linux-can@...r.kernel.org, kernel@...gutronix.de,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to
 kfree_skb() in hard IRQ context

On Tue,  3 Nov 2020 23:06:14 +0100 Marc Kleine-Budde wrote:
> From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> 
> If a driver calls can_get_echo_skb() during a hardware IRQ (which is often, but
> not always, the case), the 'WARN_ON(in_irq)' in
> net/core/skbuff.c#skb_release_head_state() might be triggered, under network
> congestion circumstances, together with the potential risk of a NULL pointer
> dereference.
> 
> The root cause of this issue is the call to kfree_skb() instead of
> dev_kfree_skb_irq() in net/core/dev.c#enqueue_to_backlog().
> 
> This patch prevents the skb to be freed within the call to netif_rx() by
> incrementing its reference count with skb_get(). The skb is finally freed by
> one of the in-irq-context safe functions: dev_consume_skb_any() or
> dev_kfree_skb_any(). The "any" version is used because some drivers might call
> can_get_echo_skb() in a normal context.
> 
> The reason for this issue to occur is that initially, in the core network
> stack, loopback skb were not supposed to be received in hardware IRQ context.
> The CAN stack is an exeption.
> 
> This bug was previously reported back in 2017 in [1] but the proposed patch
> never got accepted.
> 
> While [1] directly modifies net/core/dev.c, we try to propose here a
> smoother modification local to CAN network stack (the assumption
> behind is that only CAN devices are affected by this issue).
> 
> [1] http://lore.kernel.org/r/57a3ffb6-3309-3ad5-5a34-e93c3fe3614d@cetitec.com
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> Link: https://lore.kernel.org/r/20201002154219.4887-2-mailhol.vincent@wanadoo.fr
> Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>

Hm... Why do we receive a skb with a socket attached?

At a quick glance this is some loopback, so shouldn't we skb_orphan()
in the xmit function instead?

Otherwise we should probably fix this in enqueue_to_backlog().

> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index b70ded3760f2..73cfcd7e9517 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -538,7 +538,11 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
>  	if (!skb)
>  		return 0;
>  
> -	netif_rx(skb);
> +	skb_get(skb);
> +	if (netif_rx(skb) == NET_RX_SUCCESS)
> +		dev_consume_skb_any(skb);
> +	else
> +		dev_kfree_skb_any(skb);
>  
>  	return len;
>  }