linux-kernel - Re: [PATCH] scsi: qla2xxx: Fix memory leaks in the error handling path of 'qla2x00_mem

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:   Thu, 11 Nov 2021 12:17:12 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     njavali@...vell.com, GR-QLogic-Storage-Upstream@...vell.com,
        jejb@...ux.ibm.com, martin.petersen@...cle.com,
        gmalavali@...vell.com, hmadhani@...vell.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] scsi: qla2xxx: Fix memory leaks in the error handling
 path of 'qla2x00_mem_alloc()'

On Wed, Nov 10, 2021 at 10:11:34PM +0100, Christophe JAILLET wrote:
> In case of memory allocation failure, we should release many things and
> should not return directly.
> 
> The tricky part here, is that some (kzalloc + dma_pool_alloc) resources
> are allocated and stored in 'unusable' and a 'good' list.
> The 'good' list is then freed and only the 'unusable' list remains
> allocated.
> So, only this 'unusable' list is then freed in the error handling path of
> the function.
> 
> So, instead of adding even more code in this already huge function, just
> 'continue' (as already done if dma_pool_alloc() fails) instead of
> returning directly.
> 
> After the 'for' loop, we will then branch to the correct place of the
> error handling path when another memory allocation will (likely) fail
> afterward.
> 
> Fixes: 50b812755e97 ("scsi: qla2xxx: Fix DMA error when the DIF sg buffer crosses 4GB boundary")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> Certainly not the best solution, but look 'safe' to me.

Your analysis seems correct, but this is deeply weird.  It sort of looks
like this was debug code that was committed accidentally.  Neither
the "good" list nor the "unusable" are used except to print some debug
info:

                       ql_dbg_pci(ql_dbg_init, ha->pdev, 0x0024,
                           "%s: dif dma pool (good=%u unusable=%u)\n",
                           __func__, ha->pool.good.count,
                           ha->pool.unusable.count);

The good list is freed immediately, and then there is a no-op free in
qla2x00_mem_free().  The unusable list is preserved until qla2x00_mem_free()
but not used anywhere.

regards,
dan carpenter