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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e737866-4d59-ac2e-c53f-540912613f12@linaro.org>
Date:   Wed, 16 May 2018 09:38:35 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Douglas Anderson <dianders@...omium.org>, kishon@...com
Cc:     mgautam@...eaurora.org, mka@...omium.org,
        vivek.gautam@...eaurora.org, evgreen@...omium.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get()
 crash us



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.

thanks,
srini

> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b5b0cdc21d01..514d1dfc5630 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -936,6 +936,10 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id)
>   			return cell;
>   	}
>   
> +	/* NULL cell_id only allowed for device tree; invalid otherwise */
> +	if (!cell_id)
> +		return ERR_PTR(-EINVAL);
> +
>   	return nvmem_cell_get_from_list(cell_id);
>   }
>   EXPORT_SYMBOL_GPL(nvmem_cell_get);
>