[PATCH v1] counter: fix NULL pointer dereference on counter_comp_u8_store()

[PATCH v1] counter: fix NULL pointer dereference on counter_comp_u8_store()

[Oleksij Rempel]

Current kernel will report NULL pointer dereference with following
back trace:
  interrupt_cnt_enable_write from counter_comp_u8_store+0xc0/0xf4
  counter_comp_u8_store from dev_attr_store+0x24/0x30
  dev_attr_store from sysfs_kf_write+0x48/0x54
  sysfs_kf_write from kernfs_fop_write_iter+0x128/0x1c8
  kernfs_fop_write_iter from vfs_write+0x124/0x1b4
  vfs_write from ksys_write+0x88/0xe0
  ksys_write from sys_write+0x18/0x1c
  sys_write from ret_fast_syscall+0x0/0x1c

Add missing dev_set_drvdata() to fix it.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/counter/counter-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
index 7e0957eea094..1de16d5e9fbc 100644
--- a/drivers/counter/counter-core.c
+++ b/drivers/counter/counter-core.c
@@ -98,6 +98,8 @@ struct counter_device *counter_alloc(size_t sizeof_priv)
 	counter = &ch->counter;
 	dev = &counter->dev;
 
+	dev_set_drvdata(dev, counter);
+
 	/* Acquire unique ID */
 	err = ida_alloc(&counter_ida, GFP_KERNEL);
 	if (err < 0)
-- 
2.30.2

Re: [PATCH v1] counter: fix NULL pointer dereference on counter_comp_u8_store()

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

On Thu, Feb 03, 2022 at 02:54:18PM +0100, Oleksij Rempel wrote:
> Current kernel will report NULL pointer dereference with following
> back trace:
>   interrupt_cnt_enable_write from counter_comp_u8_store+0xc0/0xf4
>   counter_comp_u8_store from dev_attr_store+0x24/0x30
>   dev_attr_store from sysfs_kf_write+0x48/0x54
>   sysfs_kf_write from kernfs_fop_write_iter+0x128/0x1c8
>   kernfs_fop_write_iter from vfs_write+0x124/0x1b4
>   vfs_write from ksys_write+0x88/0xe0
>   ksys_write from sys_write+0x18/0x1c
>   sys_write from ret_fast_syscall+0x0/0x1c
> 
> Add missing dev_set_drvdata() to fix it.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Hi Oleksij,

This dev_set_drvdata() was removed in commit b56346ddbd82 ("counter: Use
container_of instead of drvdata to track counter_device"). It looks like
we overlooked the counter-sysfs.c file when we made that change.

Would you instead replace the dev_get_drvdata() calls in counter-sysfs.c
with respective container_of() calls? Add a Fixes tag referencing commit
b56346ddbd82 as well to your commit message.

Thanks,

William Breathitt Gray

> ---
>  drivers/counter/counter-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 7e0957eea094..1de16d5e9fbc 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -98,6 +98,8 @@ struct counter_device *counter_alloc(size_t sizeof_priv)
>  	counter = &ch->counter;
>  	dev = &counter->dev;
>  
> +	dev_set_drvdata(dev, counter);
> +
>  	/* Acquire unique ID */
>  	err = ida_alloc(&counter_ida, GFP_KERNEL);
>  	if (err < 0)
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[Uwe Kleine-König]

dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
container_of instead of drvdata to track counter_device") which wrongly
claimed there were no users of drvdata. Convert to container_of() to
fix a null pointer dereference.

Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/counter/counter-sysfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 7cc4d1d523ea..04eac41dad33 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -19,6 +19,11 @@
 
 #include "counter-sysfs.h"
 
+static inline struct counter_device *counter_from_dev(struct device *dev)
+{
+	return container_of(dev, struct counter_device, dev);
+}
+
 /**
  * struct counter_attribute - Counter sysfs attribute
  * @dev_attr:	device attribute for sysfs
@@ -90,7 +95,7 @@ static ssize_t counter_comp_u8_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	int err;
 	u8 data = 0;
 
@@ -122,7 +127,7 @@ static ssize_t counter_comp_u8_store(struct device *dev,
 				     const char *buf, size_t len)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	int err;
 	bool bool_data = 0;
 	u8 data = 0;
@@ -158,7 +163,7 @@ static ssize_t counter_comp_u32_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	const struct counter_available *const avail = a->comp.priv;
 	int err;
 	u32 data = 0;
@@ -221,7 +226,7 @@ static ssize_t counter_comp_u32_store(struct device *dev,
 				      const char *buf, size_t len)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	struct counter_count *const count = a->parent;
 	struct counter_synapse *const synapse = a->comp.priv;
 	const struct counter_available *const avail = a->comp.priv;
@@ -281,7 +286,7 @@ static ssize_t counter_comp_u64_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	int err;
 	u64 data = 0;
 
@@ -309,7 +314,7 @@ static ssize_t counter_comp_u64_store(struct device *dev,
 				      const char *buf, size_t len)
 {
 	const struct counter_attribute *const a = to_counter_attribute(attr);
-	struct counter_device *const counter = dev_get_drvdata(dev);
+	struct counter_device *const counter = counter_from_dev(dev);
 	int err;
 	u64 data = 0;
 

base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
-- 
2.34.1

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 3875 bytes --]

On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
> dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
> container_of instead of drvdata to track counter_device") which wrongly
> claimed there were no users of drvdata. Convert to container_of() to
> fix a null pointer dereference.
> 
> Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I'll pick this up and apply it to my tree.

Thanks,

William Breathitt Gray

> ---
>  drivers/counter/counter-sysfs.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
> index 7cc4d1d523ea..04eac41dad33 100644
> --- a/drivers/counter/counter-sysfs.c
> +++ b/drivers/counter/counter-sysfs.c
> @@ -19,6 +19,11 @@
>  
>  #include "counter-sysfs.h"
>  
> +static inline struct counter_device *counter_from_dev(struct device *dev)
> +{
> +	return container_of(dev, struct counter_device, dev);
> +}
> +
>  /**
>   * struct counter_attribute - Counter sysfs attribute
>   * @dev_attr:	device attribute for sysfs
> @@ -90,7 +95,7 @@ static ssize_t counter_comp_u8_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	int err;
>  	u8 data = 0;
>  
> @@ -122,7 +127,7 @@ static ssize_t counter_comp_u8_store(struct device *dev,
>  				     const char *buf, size_t len)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	int err;
>  	bool bool_data = 0;
>  	u8 data = 0;
> @@ -158,7 +163,7 @@ static ssize_t counter_comp_u32_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	const struct counter_available *const avail = a->comp.priv;
>  	int err;
>  	u32 data = 0;
> @@ -221,7 +226,7 @@ static ssize_t counter_comp_u32_store(struct device *dev,
>  				      const char *buf, size_t len)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	struct counter_count *const count = a->parent;
>  	struct counter_synapse *const synapse = a->comp.priv;
>  	const struct counter_available *const avail = a->comp.priv;
> @@ -281,7 +286,7 @@ static ssize_t counter_comp_u64_show(struct device *dev,
>  				     struct device_attribute *attr, char *buf)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	int err;
>  	u64 data = 0;
>  
> @@ -309,7 +314,7 @@ static ssize_t counter_comp_u64_store(struct device *dev,
>  				      const char *buf, size_t len)
>  {
>  	const struct counter_attribute *const a = to_counter_attribute(attr);
> -	struct counter_device *const counter = dev_get_drvdata(dev);
> +	struct counter_device *const counter = counter_from_dev(dev);
>  	int err;
>  	u64 data = 0;
>  
> 
> base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[Jarkko Nikula]

Hi

On 2/8/22 03:42, William Breathitt Gray wrote:
> On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
>> dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
>> container_of instead of drvdata to track counter_device") which wrongly
>> claimed there were no users of drvdata. Convert to container_of() to
>> fix a null pointer dereference.
>>
>> Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I'll pick this up and apply it to my tree.
> 
Perhaps late but I hit this same issue, patch here fixes it and I wanted 
to confirm it.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On Thu, Feb 17, 2022 at 04:48:50PM +0200, Jarkko Nikula wrote:
> On 2/8/22 03:42, William Breathitt Gray wrote:
> > On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
> > > dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
> > > container_of instead of drvdata to track counter_device") which wrongly
> > > claimed there were no users of drvdata. Convert to container_of() to
> > > fix a null pointer dereference.
> > > 
> > > Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > I'll pick this up and apply it to my tree.
>
> Perhaps late but I hit this same issue, patch here fixes it and I wanted to
> confirm it.
> 
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

I wonder if this patch is scheduled for 5.17. Currently it's not even in
next ... :-\

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Thu, Feb 17, 2022 at 05:03:08PM +0100, Uwe Kleine-König wrote:
> On Thu, Feb 17, 2022 at 04:48:50PM +0200, Jarkko Nikula wrote:
> > On 2/8/22 03:42, William Breathitt Gray wrote:
> > > On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
> > > > dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
> > > > container_of instead of drvdata to track counter_device") which wrongly
> > > > claimed there were no users of drvdata. Convert to container_of() to
> > > > fix a null pointer dereference.
> > > > 
> > > > Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > I'll pick this up and apply it to my tree.
> >
> > Perhaps late but I hit this same issue, patch here fixes it and I wanted to
> > confirm it.
> > 
> > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> I wonder if this patch is scheduled for 5.17. Currently it's not even in
> next ... :-\
> 
> Best regards
> Uwe

Hi Uwe,

I've got it in my tree. I'm sending a pull request for the Counter
patches for 5.17 next week, so they should all be merged after that.

Sincerely,

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

Hello,

On Fri, Feb 18, 2022 at 08:42:51AM +0900, William Breathitt Gray wrote:
> On Thu, Feb 17, 2022 at 05:03:08PM +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 17, 2022 at 04:48:50PM +0200, Jarkko Nikula wrote:
> > > On 2/8/22 03:42, William Breathitt Gray wrote:
> > > > On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
> > > > > dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
> > > > > container_of instead of drvdata to track counter_device") which wrongly
> > > > > claimed there were no users of drvdata. Convert to container_of() to
> > > > > fix a null pointer dereference.
> > > > > 
> > > > > Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > 
> > > > I'll pick this up and apply it to my tree.
> > >
> > > Perhaps late but I hit this same issue, patch here fixes it and I wanted to
> > > confirm it.
> > > 
> > > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > 
> > I wonder if this patch is scheduled for 5.17. Currently it's not even in
> > next ... :-\
> 
> I've got it in my tree. I'm sending a pull request for the Counter
> patches for 5.17 next week, so they should all be merged after that.

That's good. Still I think you could make live easier for your users to
find fixes if your tree was included in next. And in MAINTAINERS.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] counter: Stop using dev_get_drvdata() to get the counter device

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Fri, Feb 18, 2022 at 08:47:51AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Feb 18, 2022 at 08:42:51AM +0900, William Breathitt Gray wrote:
> > On Thu, Feb 17, 2022 at 05:03:08PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 17, 2022 at 04:48:50PM +0200, Jarkko Nikula wrote:
> > > > On 2/8/22 03:42, William Breathitt Gray wrote:
> > > > > On Fri, Feb 04, 2022 at 09:25:56AM +0100, Uwe Kleine-König wrote:
> > > > > > dev_get_drvdata() returns NULL since commit b56346ddbd82 ("counter: Use
> > > > > > container_of instead of drvdata to track counter_device") which wrongly
> > > > > > claimed there were no users of drvdata. Convert to container_of() to
> > > > > > fix a null pointer dereference.
> > > > > > 
> > > > > > Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > > Fixes: b56346ddbd82 ("counter: Use container_of instead of drvdata to track counter_device")
> > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > 
> > > > > I'll pick this up and apply it to my tree.
> > > >
> > > > Perhaps late but I hit this same issue, patch here fixes it and I wanted to
> > > > confirm it.
> > > > 
> > > > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > 
> > > I wonder if this patch is scheduled for 5.17. Currently it's not even in
> > > next ... :-\
> > 
> > I've got it in my tree. I'm sending a pull request for the Counter
> > patches for 5.17 next week, so they should all be merged after that.
> 
> That's good. Still I think you could make live easier for your users to
> find fixes if your tree was included in next. And in MAINTAINERS.
> 
> Best regards
> Uwe

That's a fair point, I'll update MAINTAINERS and set up my fixes branch
for inclusion in linux-next.

Incidentally, it looks like this is the only fix I have merged; I'll
submit it now by itself so we don't have to wait until next week.

Thanks,

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]