linux-kernel - Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell

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]

Message-ID: <CAD=FV=UqiipbGwMuc16DSdn9VDsU8c1K+0NBYkVS0wwLXfCOyg@mail.gmail.com>
Date:   Fri, 15 Jun 2018 08:59:52 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        Manu Gautam <mgautam@...eaurora.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Evan Green <evgreen@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get()
 crash us

Hi,

On Wed, May 16, 2018 at 1:38 AM, Srinivas Kandagatla
<srinivas.kandagatla@...aro.org> wrote:
>
>
> On 15/05/18 22:16, Douglas Anderson wrote:
>>
>> In commit ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on
>> Qcom chips") you can see a call like:
>>
>>    devm_nvmem_cell_get(dev, NULL);
>>
>> Note that the cell ID passed to the function is NULL.  This is because
>> the qcom-qusb2 driver is expected to work only on systems where the
>> PHY node is hooked up via device-tree and is nameless.
>>
>> This works OK for the most part.  The first thing nvmem_cell_get()
>> does is to call of_nvmem_cell_get() and there it's documented that a
>> NULL name is fine.  The problem happens when the call to
>> of_nvmem_cell_get() returns -EINVAL.  In such a case we'll fall back
>> to nvmem_cell_get_from_list() and eventually might (if nvmem_cells
>> isn't an empty list) crash with something that looks like:
>>
>>   strcmp
>>   nvmem_find_cell
>>   __nvmem_device_get
>>   nvmem_cell_get_from_list
>>   nvmem_cell_get
>>   devm_nvmem_cell_get
>>   qusb2_phy_probe
>>
>> There are several different ways we could fix this problem:
>>
>> One could argue that perhaps the qcom-qusb2 driver should be changed
>> to use of_nvmem_cell_get() which is allowed to have a NULL name.  In
>> that case, we'd need to add a patche to introduce
>> devm_of_nvmem_cell_get() since the qcom-qusb2 driver is using devm
>> managed resources.
>>
>> One could also argue that perhaps we could just add a name to
>> qcom-qusb2.  That would be OK but I believe it effectively changes the
>> device tree bindings, so maybe it's a no-go.
>>
>> In this patch I have chosen to fix the problem by simply not crashing
>> when a NULL cell_id is passed to nvmem_cell_get().
>>
>> NOTE: that for the qcom-qusb2 driver the "nvmem-cells" property is
>> defined to be optional and thus it's expected to be a common case that
>> we would hit this crash and this is more than just a theoretical fix.
>>
>> Fixes: ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom
>> chips")
>> Signed-off-by: Douglas Anderson <dianders@...omium.org>
>> ---
>>
>>   drivers/nvmem/core.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>
>
> Looks good to me,
> Kishon if you want to queue this one from your tree, you can add my
>
> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>
> If not I can send this via Greg's Char-misc tree.

This clearly missed the boat for 4.17, but maybe it can land for 4.18
once the merge window closes?  Is there any agreement about where it
should land?  Srinivas: do you just want to take it yourself?

Thanks!

-Doug