linux-kernel - RE: [PATCH][V2] qed: fix memory leak of a qed_spq

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: <BL2PR07MB230615AAE3CF67DEBC6083778D930@BL2PR07MB2306.namprd07.prod.outlook.com>
Date:   Wed, 21 Dec 2016 13:29:56 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Colin King <colin.king@...onical.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>,
        "Tayar, Tomer" <Tomer.Tayar@...ium.com>
Subject: RE: [PATCH][V2] qed: fix memory leak of a qed_spq_entry on error
 failure paths

> From: Colin Ian King <colin.king@...onical.com>
> 
> A qed_spq_entry entry is allocated by qed_sp_init_request but is not kfree'd
> if an error occurs, causing a memory leak. Fix this by returning the previously
> allocated spq entry and also setting *pp_ent to NULL to be safe.
> 
> Thanks to Yuval Mintz for suggestions on how to improve my original fix.
> 
> Signed-off-by: Colin Ian King <colin.king@...onical.com>

We've given it a more thorough look, and apparently this isn't the correct fix.
So I'll start by saying sorry for making you send this V2 needlessly.

It boils down to the fact there are two kinds of SPQ entries -
Those originating from the 'free_pool' and those from the 'unlimited_pending'.
Only those originating from the free_pool should be returned
using the qed_spq_return_entry(), as only those actually point to a valid
dma-mapped memory where FW expects to find the entries;
Returning the other kind would lead to assertions later,
as driver would post a ramrod to FW which actually points to address 0.

Looking at the error flows, it seems possible this isn't the only faulty
error flow in the SPQ. I suggest you'd drop this and we'll take it from
here [although if you really have the urge to continue - please do].

Thanks,
Yuval