[PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us

[PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us

[Douglas Anderson]

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@chromium.org>
---

 drivers/nvmem/core.c | 4 ++++
 1 file changed, 4 insertions(+)

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);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us

[Srinivas Kandagatla]

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@chromium.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@linaro.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);
> 

Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us

[Doug Anderson]

Hi,

On Wed, May 16, 2018 at 1:38 AM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.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@chromium.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@linaro.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

[PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us

[Srinivas Kandagatla]

From: Douglas Anderson <dianders@chromium.org>

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@chromium.org>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Hi Greg, 
Can you please pick this one for next possible rc.

thanks,
srini

 drivers/nvmem/core.c | 4 ++++
 1 file changed, 4 insertions(+)

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);
-- 
2.16.2