netdev - Re: [PATCH net 1/2] net: sfc: ef10: fix memory leak in efx_ef10_mtd

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: <20220511060954.byiw5sem4vkwhjhr@gmail.com>
Date:   Wed, 11 May 2022 07:09:54 +0100
From:   Martin Habets <habetsm.xilinx@...il.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, ecree.xilinx@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: sfc: ef10: fix memory leak in
 efx_ef10_mtd_probe()

Hi Taehee,

On Tue, May 10, 2022 at 03:36:18PM +0000, Taehee Yoo wrote:
> In the NIC ->probe() callback, ->mtd_probe() callback is called.
> If NIC has 2 ports, ->probe() is called twice and ->mtd_probe() too.
> In the ->mtd_probe(), which is efx_ef10_mtd_probe() it allocates and
> initializes mtd partiion.
> But mtd partition for sfc is shared data.
> So that allocated mtd partition data from last called
> efx_ef10_mtd_probe() will not be used.
> Therefore it must be freed.
> But it doesn't free a not used mtd partition data in efx_ef10_mtd_probe().

Your analysis is correct for X2, where the 2nd port does not have
any MTD partitions.
Note that on 7000 series the 2nd (and possibly 3rd and 4th) port do have
MTD partitions. On those efx_mtd_remove() will free the data.

On X2 efx_mtd_remove() returns too early for the 2nd port due to:
        if (list_empty(&efx->mtd_list))
                return;

I don't see an easy way to fix that code, since it does not know
the memory address. So your fix is the best we can do.

> 
> kmemleak reports:
> unreferenced object 0xffff88811ddb0000 (size 63168):
>   comm "systemd-udevd", pid 265, jiffies 4294681048 (age 348.586s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<ffffffffa3767749>] kmalloc_order_trace+0x19/0x120
>     [<ffffffffa3873f0e>] __kmalloc+0x20e/0x250
>     [<ffffffffc041389f>] efx_ef10_mtd_probe+0x11f/0x270 [sfc]
>     [<ffffffffc0484c8a>] efx_pci_probe.cold.17+0x3df/0x53d [sfc]
>     [<ffffffffa414192c>] local_pci_probe+0xdc/0x170
>     [<ffffffffa4145df5>] pci_device_probe+0x235/0x680
>     [<ffffffffa443dd52>] really_probe+0x1c2/0x8f0
>     [<ffffffffa443e72b>] __driver_probe_device+0x2ab/0x460
>     [<ffffffffa443e92a>] driver_probe_device+0x4a/0x120
>     [<ffffffffa443f2ae>] __driver_attach+0x16e/0x320
>     [<ffffffffa4437a90>] bus_for_each_dev+0x110/0x190
>     [<ffffffffa443b75e>] bus_add_driver+0x39e/0x560
>     [<ffffffffa4440b1e>] driver_register+0x18e/0x310
>     [<ffffffffc02e2055>] 0xffffffffc02e2055
>     [<ffffffffa3001af3>] do_one_initcall+0xc3/0x450
>     [<ffffffffa33ca574>] do_init_module+0x1b4/0x700
> 
> Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
> Signed-off-by: Taehee Yoo <ap420073@...il.com>

Acked-by: Martin Habets <habetsm.xilinx@...il.com>

> ---
>  drivers/net/ethernet/sfc/ef10.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 50d535981a35..f8edb3f1b73a 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
>  		n_parts++;
>  	}
>  
> +	if (!n_parts) {
> +		kfree(parts);
> +		return 0;
> +	}
> +
>  	rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
>  fail:
>  	if (rc)
> -- 
> 2.17.1