netdev - Re: [PATCH net] net: qlogic: Fix error paths in ql_alloc_large

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]

Date:   Tue, 17 Dec 2019 22:07:26 -0800 (PST)
From:   David Miller <davem@...emloft.net>
To:     ben@...adent.org.uk
Cc:     netdev@...r.kernel.org, GR-Linux-NIC-Dev@...vell.com,
        navid.emamdoost@...il.com
Subject: Re: [PATCH net] net: qlogic: Fix error paths in
 ql_alloc_large_buffers()

From: Ben Hutchings <ben@...adent.org.uk>
Date: Tue, 17 Dec 2019 01:57:40 +0000

> ql_alloc_large_buffers() has the usual RX buffer allocation
> loop where it allocates skbs and maps them for DMA.  It also
> treats failure as a fatal error.
> 
> There are (at least) three bugs in the error paths:
> 
> 1. ql_free_large_buffers() assumes that the lrg_buf[] entry for the
> first buffer that couldn't be allocated will have .skb == NULL.
> But the qla_buf[] array is not zero-initialised.
> 
> 2. ql_free_large_buffers() DMA-unmaps all skbs in lrg_buf[].  This is
> incorrect for the last allocated skb, if DMA mapping failed.
> 
> 3. Commit 1acb8f2a7a9f ("net: qlogic: Fix memory leak in
> ql_alloc_large_buffers") added a direct call to dev_kfree_skb_any()
> after the skb is recorded in lrg_buf[], so ql_free_large_buffers()
> will double-free it.
> 
> The bugs are somewhat inter-twined, so fix them all at once:
> 
> * Clear each entry in qla_buf[] before attempting to allocate
>   an skb for it.  This goes half-way to fixing bug 1.
> * Set the .skb field only after the skb is DMA-mapped.  This
>   fixes the rest.
> 
> Fixes: 1357bfcf7106 ("qla3xxx: Dynamically size the rx buffer queue ...")
> Fixes: 0f8ab89e825f ("qla3xxx: Check return code from pci_map_single() ...")
> Fixes: 1acb8f2a7a9f ("net: qlogic: Fix memory leak in ql_alloc_large_buffers")
> Signed-off-by: Ben Hutchings <ben@...adent.org.uk>

I've been over this a few times, applied and queued up for -stable.

Thanks.