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] [day] [month] [year] [list]
Message-ID: <20220223050010.GA20891@hu-pkondeti-hyd.qualcomm.com>
Date:   Wed, 23 Feb 2022 10:30:10 +0530
From:   Pavan Kondeti <quic_pkondeti@...cinc.com>
To:     Daehwan Jung <dh10.jung@...sung.com>
CC:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Muchun Song <songmuchun@...edance.com>,
        "Andrew Morton" <akpm@...ux-foundation.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        <linux-usb@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: usb: gadget: rndis: add spinlock for rndis response list

On Tue, Feb 22, 2022 at 02:29:28PM +0900, Daehwan Jung wrote:
> There's no lock for rndis response list. It could cause list corruption
> if there're two different list_add at the same time like below.
> It's better to add in rndis_add_response / rndis_free_response
> / rndis_get_next_response to prevent any race condition on response list.
> 
> [  361.894299] [1:   irq/191-dwc3:16979] list_add corruption.
> next->prev should be prev (ffffff80651764d0),
> but was ffffff883dc36f80. (next=ffffff80651764d0).
> 
> [  361.904380] [1:   irq/191-dwc3:16979] Call trace:
> [  361.904391] [1:   irq/191-dwc3:16979]  __list_add_valid+0x74/0x90
> [  361.904401] [1:   irq/191-dwc3:16979]  rndis_msg_parser+0x168/0x8c0
> [  361.904409] [1:   irq/191-dwc3:16979]  rndis_command_complete+0x24/0x84
> [  361.904417] [1:   irq/191-dwc3:16979]  usb_gadget_giveback_request+0x20/0xe4
> [  361.904426] [1:   irq/191-dwc3:16979]  dwc3_gadget_giveback+0x44/0x60
> [  361.904434] [1:   irq/191-dwc3:16979]  dwc3_ep0_complete_data+0x1e8/0x3a0
> [  361.904442] [1:   irq/191-dwc3:16979]  dwc3_ep0_interrupt+0x29c/0x3dc
> [  361.904450] [1:   irq/191-dwc3:16979]  dwc3_process_event_entry+0x78/0x6cc
> [  361.904457] [1:   irq/191-dwc3:16979]  dwc3_process_event_buf+0xa0/0x1ec
> [  361.904465] [1:   irq/191-dwc3:16979]  dwc3_thread_interrupt+0x34/0x5c
> 

This is just one context. what about the other contexts? Interested to see the
different contexts under which this list is being manipulated. From the
f_rndis perspective, this  list is touched from setup and disconnect
callbacks. so are those two contexts not serialized from the UDC? not saying
we don't need lock, but would be good to record that information in the change
log.

> Fixes: f6281af9d62e ("usb: gadget: rndis: use list_for_each_entry_safe")
> Signed-off-by: Daehwan Jung <dh10.jung@...sung.com>
> ---
>  drivers/usb/gadget/function/rndis.c | 8 ++++++++
>  drivers/usb/gadget/function/rndis.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index 431d5a7..79fd994 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -919,6 +919,7 @@ struct rndis_params *rndis_register(void (*resp_avail)(void *v), void *v)
>  	params->resp_avail = resp_avail;
>  	params->v = v;
>  	INIT_LIST_HEAD(&params->resp_queue);
> +	spin_lock_init(&params->resp_lock);
>  	pr_debug("%s: configNr = %d\n", __func__, i);
>  
>  	return params;
> @@ -1012,12 +1013,14 @@ void rndis_free_response(struct rndis_params *params, u8 *buf)
>  {
>  	rndis_resp_t *r, *n;
>  
> +	spin_lock(&params->resp_lock);
>  	list_for_each_entry_safe(r, n, &params->resp_queue, list) {
>  		if (r->buf == buf) {
>  			list_del(&r->list);
>  			kfree(r);
>  		}
>  	}
> +	spin_unlock(&params->resp_lock);
>  }
>  EXPORT_SYMBOL_GPL(rndis_free_response);

Are you sure that this lock is not acquired from any interrupt context from
other contexts? Also would it be true for all UDC? some UDC may call setup
from hadirq context and disconnect in process context etc or vice versa. we
don't want to acquire lock without disabling interrupts in that case.

Thanks,
Pavan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ