LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us
@ 2018-05-15 21:16 Douglas Anderson
  2018-05-16  8:38 ` Srinivas Kandagatla
  0 siblings, 1 reply; 4+ messages in thread
From: Douglas Anderson @ 2018-05-15 21:16 UTC (permalink / raw)
  To: srinivas.kandagatla, kishon
  Cc: mgautam, mka, vivek.gautam, evgreen, Douglas Anderson, linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us
  2018-05-15 21:16 [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us Douglas Anderson
@ 2018-05-16  8:38 ` Srinivas Kandagatla
  2018-06-15 15:59   ` Doug Anderson
  0 siblings, 1 reply; 4+ messages in thread
From: Srinivas Kandagatla @ 2018-05-16  8:38 UTC (permalink / raw)
  To: Douglas Anderson, kishon
  Cc: mgautam, mka, vivek.gautam, evgreen, linux-kernel



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us
  2018-05-16  8:38 ` Srinivas Kandagatla
@ 2018-06-15 15:59   ` Doug Anderson
  0 siblings, 0 replies; 4+ messages in thread
From: Doug Anderson @ 2018-06-15 15:59 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Kishon Vijay Abraham I, Manu Gautam, Matthias Kaehlcke,
	Vivek Gautam, Evan Green, LKML

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us
@ 2018-06-18 17:30 Srinivas Kandagatla
  0 siblings, 0 replies; 4+ messages in thread
From: Srinivas Kandagatla @ 2018-06-18 17:30 UTC (permalink / raw)
  To: gregkh; +Cc: linux-kernel, srinivas.kandagatla, Douglas Anderson

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-06-18 17:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-15 21:16 [PATCH] nvmem: Don't let a NULL cell_id for nvmem_cell_get() crash us Douglas Anderson
2018-05-16  8:38 ` Srinivas Kandagatla
2018-06-15 15:59   ` Doug Anderson
2018-06-18 17:30 Srinivas Kandagatla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).