netdev - Re: [RFC] can: can_create_echo_skb(): fix echo skb generation: always use skb

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: <f2ae9b3a-0d10-64ae-1533-2308e9346ebc@pengutronix.de>
Date:   Fri, 16 Oct 2020 21:36:08 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Oleksij Rempel <o.rempel@...gutronix.de>,
        dev.kurt@...dijck-laurijssen.be, wg@...ndegger.com
Cc:     netdev@...r.kernel.org, kernel@...gutronix.de,
        linux-can@...r.kernel.org
Subject: Re: [RFC] can: can_create_echo_skb(): fix echo skb generation: always
 use skb_clone()

On 2/14/20 1:09 PM, Oleksij Rempel wrote:
> Hi all,
> 
> any comments on this patch?

I'm going to take this patch now for 5.10....Comments?

Marc

> 
> On Fri, Jan 24, 2020 at 02:26:56PM +0100, Oleksij Rempel wrote:
>> All user space generated SKBs are owned by a socket (unless injected
>> into the key via AF_PACKET). If a socket is closed, all associated skbs
>> will be cleaned up.
>>
>> This leads to a problem when a CAN driver calls can_put_echo_skb() on a
>> unshared SKB. If the socket is closed prior to the TX complete handler,
>> can_get_echo_skb() and the subsequent delivering of the echo SKB to
>> all registered callbacks, a SKB with a refcount of 0 is delivered.
>>
>> To avoid the problem, in can_get_echo_skb() the original SKB is now
>> always cloned, regardless of shared SKB or not. If the process exists it
>> can now safely discard its SKBs, without disturbing the delivery of the
>> echo SKB.
>>
>> The problem shows up in the j1939 stack, when it clones the
>> incoming skb, which detects the already 0 refcount.
>>
>> We can easily reproduce this with following example:
>>
>> testj1939 -B -r can0: &
>> cansend can0 1823ff40#0123
>>
>> WARNING: CPU: 0 PID: 293 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
>> refcount_t: addition on 0; use-after-free.
>> Modules linked in: coda_vpu imx_vdoa videobuf2_vmalloc dw_hdmi_ahb_audio vcan
>> CPU: 0 PID: 293 Comm: cansend Not tainted 5.5.0-rc6-00376-g9e20dcb7040d #1
>> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>> Backtrace:
>> [<c010f570>] (dump_backtrace) from [<c010f90c>] (show_stack+0x20/0x24)
>> [<c010f8ec>] (show_stack) from [<c0c3e1a4>] (dump_stack+0x8c/0xa0)
>> [<c0c3e118>] (dump_stack) from [<c0127fec>] (__warn+0xe0/0x108)
>> [<c0127f0c>] (__warn) from [<c01283c8>] (warn_slowpath_fmt+0xa8/0xcc)
>> [<c0128324>] (warn_slowpath_fmt) from [<c0539c0c>] (refcount_warn_saturate+0x108/0x174)
>> [<c0539b04>] (refcount_warn_saturate) from [<c0ad2cac>] (j1939_can_recv+0x20c/0x210)
>> [<c0ad2aa0>] (j1939_can_recv) from [<c0ac9dc8>] (can_rcv_filter+0xb4/0x268)
>> [<c0ac9d14>] (can_rcv_filter) from [<c0aca2cc>] (can_receive+0xb0/0xe4)
>> [<c0aca21c>] (can_receive) from [<c0aca348>] (can_rcv+0x48/0x98)
>> [<c0aca300>] (can_rcv) from [<c09b1fdc>] (__netif_receive_skb_one_core+0x64/0x88)
>> [<c09b1f78>] (__netif_receive_skb_one_core) from [<c09b2070>] (__netif_receive_skb+0x38/0x94)
>> [<c09b2038>] (__netif_receive_skb) from [<c09b2130>] (netif_receive_skb_internal+0x64/0xf8)
>> [<c09b20cc>] (netif_receive_skb_internal) from [<c09b21f8>] (netif_receive_skb+0x34/0x19c)
>> [<c09b21c4>] (netif_receive_skb) from [<c0791278>] (can_rx_offload_napi_poll+0x58/0xb4)
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
>> ---
>>  include/linux/can/skb.h | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>> index a954def26c0d..0783b0c6d9e2 100644
>> --- a/include/linux/can/skb.h
>> +++ b/include/linux/can/skb.h
>> @@ -61,21 +61,17 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
>>   */
>>  static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
>>  {
>> -	if (skb_shared(skb)) {
>> -		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>> +	struct sk_buff *nskb;
>>  
>> -		if (likely(nskb)) {
>> -			can_skb_set_owner(nskb, skb->sk);
>> -			consume_skb(skb);
>> -			return nskb;
>> -		} else {
>> -			kfree_skb(skb);
>> -			return NULL;
>> -		}
>> +	nskb = skb_clone(skb, GFP_ATOMIC);
>> +	if (unlikely(!nskb)) {
>> +		kfree_skb(skb);
>> +		return NULL;
>>  	}
>>  
>> -	/* we can assume to have an unshared skb with proper owner */
>> -	return skb;
>> +	can_skb_set_owner(nskb, skb->sk);
>> +	consume_skb(skb);
>> +	return nskb;
>>  }
>>  
>>  #endif /* !_CAN_SKB_H */
>> -- 
>> 2.25.0
>>
>>
>>
> 


-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)