netdev - Re: [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in qrtr_port

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]

Date:   Fri, 26 Mar 2021 10:31:57 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Du Cheng <ducheng2@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Shuah Khan <skhan@...uxfoundation.org>,
        syzbot+3eec59e770685e3dc879@...kaller.appspotmail.com
Subject: Re: [PATCH] net:qrtr: fix allocator flag of idr_alloc_u32() in
 qrtr_port_assign()

On Fri, Mar 26, 2021 at 11:33:45AM +0800, Du Cheng wrote:
> change the allocator flag of idr_alloc_u32 from GFP_ATOMIC to
> GFP_KERNEL, as GFP_ATOMIC caused BUG: "using smp_processor_id() in
> preemptible" as reported by syzkaller.
> 
> Reported-by: syzbot+3eec59e770685e3dc879@...kaller.appspotmail.com
> Signed-off-by: Du Cheng <ducheng2@...il.com>
> ---
> Hi David & Jakub,
> 
> Although this is a simple fix to make syzkaller happy, I feel that maybe a more
> proper fix is to convert qrtr_ports from using IDR to radix_tree (which is in
> fact xarray) ? 
> 
> I found some previous work done in 2019 by Matthew Wilcox:
> https://lore.kernel.org/netdev/20190820223259.22348-1-willy@infradead.org/t/#mcb60ad4c34e35a6183c7353c8a44ceedfcff297d
> but that was not merged as of now. My wild guess is that it was probably
> in conflicti with the conversion of radix_tree to xarray during 2020, and that
> might cause the direct use of xarray in qrtr.c unfavorable.
> 
> Shall I proceed with converting qrtr_pors to use radix_tree (or just xarray)?

Try it and see.  But how would that resolve this issue?  Those other
structures would also need to allocate memory at this point in time and
you need to tell it if it can sleep or not.

> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index edb6ac17ceca..ee42e1e1d4d4 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -722,17 +722,17 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
>  	mutex_lock(&qrtr_port_lock);
>  	if (!*port) {
>  		min_port = QRTR_MIN_EPH_SOCKET;
> -		rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
> +		rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_KERNEL);

Are you sure that you can sleep in this code path?

thanks,

greg k-h