Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 18 May 2021 22:02:27 +0200
From:   Michael Walle <michael@walle.cc>
To:     Jon Hunter <jonathanh@nvidia.com>
Cc:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-tegra@vger.kernel.org
Subject: Re: [PATCH] mtd: core: Fix freeing of otp_info buffer
In-Reply-To: <20210518185503.162787-1-jonathanh@nvidia.com>
References: <20210424110608.15748-6-michael@walle.cc>
 <20210518185503.162787-1-jonathanh@nvidia.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <016ead00625f91d1247190e7c68c2086@walle.cc>
Precedence: bulk

Am 2021-05-18 20:55, schrieb Jon Hunter:
> Commit 4b361cfa8624 ("mtd: core: add OTP nvmem provider support") is
> causing the following panic ...
> 
>  ------------[ cut here ]------------
>  kernel BUG at /local/workdir/tegra/linux_next/kernel/mm/slab.c:2730!
>  Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>  Modules linked in:
>  CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.13.0-rc2-next-20210518 #1
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  PC is at ___cache_free+0x3f8/0x51c
>  ...
>  [<c029bb1c>] (___cache_free) from [<c029c658>] (kfree+0xac/0x1bc)
>  [<c029c658>] (kfree) from [<c06da094>] (mtd_otp_size+0xc4/0x108)
>  [<c06da094>] (mtd_otp_size) from [<c06dc864>]
> (mtd_device_parse_register+0xe4/0x2b4)
>  [<c06dc864>] (mtd_device_parse_register) from [<c06e3ccc>]
> (spi_nor_probe+0x210/0x2c0)
>  [<c06e3ccc>] (spi_nor_probe) from [<c06e9578>] (spi_probe+0x88/0xac)
>  [<c06e9578>] (spi_probe) from [<c066891c>] (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] 
> (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0666cf8>]
> (bus_for_each_drv+0x5c/0xbc)
>  [<c0666cf8>] (bus_for_each_drv) from [<c0668694>] 
> (__device_attach+0xe4/0x150)
>  [<c0668694>] (__device_attach) from [<c06679e0>] 
> (bus_probe_device+0x84/0x8c)
>  [<c06679e0>] (bus_probe_device) from [<c06657f8>] 
> (device_add+0x48c/0x868)
>  [<c06657f8>] (device_add) from [<c06eb784>] 
> (spi_add_device+0xa0/0x168)
>  [<c06eb784>] (spi_add_device) from [<c06ec9a8>]
> (spi_register_controller+0x8b8/0xb38)
>  [<c06ec9a8>] (spi_register_controller) from [<c06ecc3c>]
> (devm_spi_register_controller+0x14/0x50)
>  [<c06ecc3c>] (devm_spi_register_controller) from [<c06f0510>]
> (tegra_spi_probe+0x33c/0x450)
>  [<c06f0510>] (tegra_spi_probe) from [<c066abec>] 
> (platform_probe+0x5c/0xb8)
>  [<c066abec>] (platform_probe) from [<c066891c>] 
> (really_probe+0x214/0x3a4)
>  [<c066891c>] (really_probe) from [<c0668b14>] 
> (driver_probe_device+0x68/0xc0)
>  [<c0668b14>] (driver_probe_device) from [<c0668e30>]
> (device_driver_attach+0x58/0x60)
>  [<c0668e30>] (device_driver_attach) from [<c0668eb8>]
> (__driver_attach+0x80/0xc8)
>  [<c0668eb8>] (__driver_attach) from [<c0666c48>] 
> (bus_for_each_dev+0x78/0xb8)
>  [<c0666c48>] (bus_for_each_dev) from [<c0667c44>] 
> (bus_add_driver+0x164/0x1e8)
>  [<c0667c44>] (bus_add_driver) from [<c066997c>] 
> (driver_register+0x7c/0x114)
>  [<c066997c>] (driver_register) from [<c010223c>] 
> (do_one_initcall+0x50/0x2b0)
>  [<c010223c>] (do_one_initcall) from [<c11011f0>]
> (kernel_init_freeable+0x1a8/0x1fc)
>  [<c11011f0>] (kernel_init_freeable) from [<c0c09190>] 
> (kernel_init+0x8/0x118)
>  [<c0c09190>] (kernel_init) from [<c01001b0>] (ret_from_fork+0x14/0x24)
>  ...
>  ---[ end trace 0f652dd222de75d7 ]---
> 
> In the function mtd_otp_size() a buffer is allocated by calling
> kmalloc() and a pointer to the buffer is stored in a variable 'info'.
> The pointer 'info' may then be incremented depending on the length
> returned from mtd_get_user/fact_prot_info(). If 'info' is incremented,
> when kfree() is called to free the buffer the above panic occurs 
> because
> we are no longer passing the original address of the buffer allocated.
> Fix this by indexing through the buffer allocated to avoid incrementing
> the pointer.
> 
> Fixes: 4b361cfa8624 ("mtd: core: add OTP nvmem provider support")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

uhm.. yes of course. Two fixes for this function. Not my best day :/

I'm wondering why CONFIG_SLUB_DEBUG_ON doesn't catch this, whereas
slub_debug=f (or fzpu) as commandline parameter works as expected.

Reviewed-by: Michael Walle <michael@walle.cc>

Thanks,
-michael