[PATCH 0/3] driver core: class: Fix bug and code improvements for class APIs

[PATCH 0/3] driver core: class: Fix bug and code improvements for class APIs

[Zijun Hu]

This patch series is to

- Fix an potential wild pointer dereference bug for API:
  class_dev_iter_next()

- Improve the following APIs:
  class_for_each_device()
  class_find_device()

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (3):
      driver core: class: Fix wild pointer dereference in API class_dev_iter_next()
      driver core: class: Correct WARN() message in APIs class_(for_each|find)_device()
      driver core: class: Delete a redundant check in APIs class_(for_each|find)_device()

 drivers/base/class.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
---
base-commit: 9bd133f05b1dca5ca4399a76d04d0f6f4d454e44
change-id: 20241104-class_fix-f176bd9eba22

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
has return type void, but it does not initialize its output parameter @iter
when suffers class_to_subsys(@class) error, so caller can not detect the
error and call API class_dev_iter_next(@iter) which will dereference wild
pointers of @iter's members as shown by below typical usage:

// @iter's members are wild pointers
struct class_dev_iter iter;

// No change in @iter when the error happens.
class_dev_iter_init(&iter, ...);

// dereference these wild member pointers here.
while (dev = class_dev_iter_next(&iter)) { ... }.

Actually, all callers of the API have such usage pattern in kernel tree.
Fix by memset() @iter in API *_init() and error checking @iter in *_next().

Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

---
Alternative fix solutions ever thought about:

1) Use BUG_ON(!sp) instead of error return in class_dev_iter_init().
2) Change class_dev_iter_init()'s type to int, lots of jobs to do.
---
 drivers/base/class.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index cb5359235c70..b331dda002e3 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -323,8 +323,11 @@ void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
 	struct subsys_private *sp = class_to_subsys(class);
 	struct klist_node *start_knode = NULL;
 
-	if (!sp)
+	memset(iter, 0, sizeof(*iter));
+	if (!sp) {
+		pr_crit("%s: the class was not registered yet\n", __func__);
 		return;
+	}
 
 	if (start)
 		start_knode = &start->p->knode_class;
@@ -351,6 +354,9 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
 	struct klist_node *knode;
 	struct device *dev;
 
+	if (!iter->sp)
+		return NULL;
+
 	while (1) {
 		knode = klist_next(&iter->ki);
 		if (!knode)

-- 
2.34.1

Re: [PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Greg Kroah-Hartman]

On Tue, Nov 05, 2024 at 08:20:22AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
> has return type void, but it does not initialize its output parameter @iter
> when suffers class_to_subsys(@class) error, so caller can not detect the
> error and call API class_dev_iter_next(@iter) which will dereference wild
> pointers of @iter's members as shown by below typical usage:
> 
> // @iter's members are wild pointers
> struct class_dev_iter iter;
> 
> // No change in @iter when the error happens.
> class_dev_iter_init(&iter, ...);
> 
> // dereference these wild member pointers here.
> while (dev = class_dev_iter_next(&iter)) { ... }.
> 
> Actually, all callers of the API have such usage pattern in kernel tree.
> Fix by memset() @iter in API *_init() and error checking @iter in *_next().
> 
> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
> Cc: stable@vger.kernel.org

There is no in-kernel broken users of this from what I can tell, right?
Otherwise things would have blown up by now, so why is this needed in
stable kernels?

thanks,

greg k-h

Re: [PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Zijun Hu]

On 2024/11/12 19:43, Greg Kroah-Hartman wrote:
> On Tue, Nov 05, 2024 at 08:20:22AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
>> has return type void, but it does not initialize its output parameter @iter
>> when suffers class_to_subsys(@class) error, so caller can not detect the
>> error and call API class_dev_iter_next(@iter) which will dereference wild
>> pointers of @iter's members as shown by below typical usage:
>>
>> // @iter's members are wild pointers
>> struct class_dev_iter iter;
>>
>> // No change in @iter when the error happens.
>> class_dev_iter_init(&iter, ...);
>>
>> // dereference these wild member pointers here.
>> while (dev = class_dev_iter_next(&iter)) { ... }.
>>
>> Actually, all callers of the API have such usage pattern in kernel tree.
>> Fix by memset() @iter in API *_init() and error checking @iter in *_next().
>>
>> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
>> Cc: stable@vger.kernel.org
> 
> There is no in-kernel broken users of this from what I can tell, right?
> Otherwise things would have blown up by now, so why is this needed in
> stable kernels?
> 

For all callers of the API in current kernel tree, the class should have
been registered successfully when the API is invoking.

so, could you remove both Fix and stable tag directly?

> thanks,
> 
> greg k-h

Re: [PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Greg Kroah-Hartman]

On Tue, Nov 12, 2024 at 10:46:27PM +0800, Zijun Hu wrote:
> On 2024/11/12 19:43, Greg Kroah-Hartman wrote:
> > On Tue, Nov 05, 2024 at 08:20:22AM +0800, Zijun Hu wrote:
> >> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>
> >> class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
> >> has return type void, but it does not initialize its output parameter @iter
> >> when suffers class_to_subsys(@class) error, so caller can not detect the
> >> error and call API class_dev_iter_next(@iter) which will dereference wild
> >> pointers of @iter's members as shown by below typical usage:
> >>
> >> // @iter's members are wild pointers
> >> struct class_dev_iter iter;
> >>
> >> // No change in @iter when the error happens.
> >> class_dev_iter_init(&iter, ...);
> >>
> >> // dereference these wild member pointers here.
> >> while (dev = class_dev_iter_next(&iter)) { ... }.
> >>
> >> Actually, all callers of the API have such usage pattern in kernel tree.
> >> Fix by memset() @iter in API *_init() and error checking @iter in *_next().
> >>
> >> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
> >> Cc: stable@vger.kernel.org
> > 
> > There is no in-kernel broken users of this from what I can tell, right?
> > Otherwise things would have blown up by now, so why is this needed in
> > stable kernels?
> > 
> 
> For all callers of the API in current kernel tree, the class should have
> been registered successfully when the API is invoking.

Great, so the existing code is just fine :)

> so, could you remove both Fix and stable tag directly?

Nope, sorry.  Asking a maintainer that gets hundreds of patches to
hand-edit them does not scale.

But really, as all in-kernel users are just fine, why add additional
code if it's not needed?  THat's just going to increase our maintance
burden for the next 40+ years for no good reason.

thanks,

greg k-h

Re: [PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Zijun Hu]

On 2024/11/12 22:57, Greg Kroah-Hartman wrote:
> On Tue, Nov 12, 2024 at 10:46:27PM +0800, Zijun Hu wrote:
>> On 2024/11/12 19:43, Greg Kroah-Hartman wrote:
>>> On Tue, Nov 05, 2024 at 08:20:22AM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
>>>> has return type void, but it does not initialize its output parameter @iter
>>>> when suffers class_to_subsys(@class) error, so caller can not detect the
>>>> error and call API class_dev_iter_next(@iter) which will dereference wild
>>>> pointers of @iter's members as shown by below typical usage:
>>>>
>>>> // @iter's members are wild pointers
>>>> struct class_dev_iter iter;
>>>>
>>>> // No change in @iter when the error happens.
>>>> class_dev_iter_init(&iter, ...);
>>>>
>>>> // dereference these wild member pointers here.
>>>> while (dev = class_dev_iter_next(&iter)) { ... }.
>>>>
>>>> Actually, all callers of the API have such usage pattern in kernel tree.
>>>> Fix by memset() @iter in API *_init() and error checking @iter in *_next().
>>>>
>>>> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
>>>> Cc: stable@vger.kernel.org
>>>
>>> There is no in-kernel broken users of this from what I can tell, right?
>>> Otherwise things would have blown up by now, so why is this needed in
>>> stable kernels?
>>>
>>
>> For all callers of the API in current kernel tree, the class should have
>> been registered successfully when the API is invoking.
> 
> Great, so the existing code is just fine :)
> 
>> so, could you remove both Fix and stable tag directly?
> 
> Nope, sorry.  Asking a maintainer that gets hundreds of patches to
> hand-edit them does not scale.
>
okay, let me send a updated revision now.

> But really, as all in-kernel users are just fine, why add additional
> code if it's not needed?  THat's just going to increase our maintance
> burden for the next 40+ years for no good reason.
> 

IMO, this fix is very necessary for the API.

> thanks,
> 
> greg k-h

Re: [PATCH 1/3] driver core: class: Fix wild pointer dereference in API class_dev_iter_next()

[Zijun Hu]

On 2024/11/12 22:57, Greg Kroah-Hartman wrote:
> On Tue, Nov 12, 2024 at 10:46:27PM +0800, Zijun Hu wrote:
>> On 2024/11/12 19:43, Greg Kroah-Hartman wrote:
>>> On Tue, Nov 05, 2024 at 08:20:22AM +0800, Zijun Hu wrote:
>>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>
>>>> class_dev_iter_init(struct class_dev_iter *iter, struct class *class, ...)
>>>> has return type void, but it does not initialize its output parameter @iter
>>>> when suffers class_to_subsys(@class) error, so caller can not detect the
>>>> error and call API class_dev_iter_next(@iter) which will dereference wild
>>>> pointers of @iter's members as shown by below typical usage:
>>>>
>>>> // @iter's members are wild pointers
>>>> struct class_dev_iter iter;
>>>>
>>>> // No change in @iter when the error happens.
>>>> class_dev_iter_init(&iter, ...);
>>>>
>>>> // dereference these wild member pointers here.
>>>> while (dev = class_dev_iter_next(&iter)) { ... }.
>>>>
>>>> Actually, all callers of the API have such usage pattern in kernel tree.
>>>> Fix by memset() @iter in API *_init() and error checking @iter in *_next().
>>>>
>>>> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
>>>> Cc: stable@vger.kernel.org
>>>
>>> There is no in-kernel broken users of this from what I can tell, right?
>>> Otherwise things would have blown up by now, so why is this needed in
>>> stable kernels?
>>>
>>
>> For all callers of the API in current kernel tree, the class should have
>> been registered successfully when the API is invoking.
> 
> Great, so the existing code is just fine :)
> 
>> so, could you remove both Fix and stable tag directly?
> 
> Nope, sorry.  Asking a maintainer that gets hundreds of patches to
> hand-edit them does not scale.
> 
> But really, as all in-kernel users are just fine, why add additional
> code if it's not needed?  THat's just going to increase our maintance
> burden for the next 40+ years for no good reason.
> 

Hi Greg.

This fix is very worthy and necessary since it fixes the APIs self issue
and the issue is irrelevant with how various API users (in-tree or
out-of-tree) use it as below inference shown:

API class_dev_iter_init() has checks for class_to_subsys(class) error
-> the error may happen
-> once the error happens
-> wild pointers dereference must happen within API class_dev_iter_next()
-> how terrible for such issue, and the error handling also have no
prompt messages.

API users must not like silent wild pointer dereference once the error
happen.

> thanks,
> 
> greg k-h

[PATCH 2/3] driver core: class: Correct WARN() message in APIs class_(for_each|find)_device()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For both API class_for_each_device(const struct class *class, ...) and
class_find_device(const struct class *class, ...), their WARN() messages
prompt @class was not initialized when suffer class_to_subsys(@class)
error, but the error actually means @class was not registered, so these
warning messages are not accurate.

Fix by replacing term initialized with registered within these messages.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/class.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index b331dda002e3..e81da280af74 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -411,7 +411,7 @@ int class_for_each_device(const struct class *class, const struct device *start,
 	if (!class)
 		return -EINVAL;
 	if (!sp) {
-		WARN(1, "%s called for class '%s' before it was initialized",
+		WARN(1, "%s called for class '%s' before it was registered",
 		     __func__, class->name);
 		return -EINVAL;
 	}
@@ -459,7 +459,7 @@ struct device *class_find_device(const struct class *class, const struct device
 	if (!class)
 		return NULL;
 	if (!sp) {
-		WARN(1, "%s called for class '%s' before it was initialized",
+		WARN(1, "%s called for class '%s' before it was registered",
 		     __func__, class->name);
 		return NULL;
 	}

-- 
2.34.1

[PATCH 3/3] driver core: class: Delete a redundant check in APIs class_(for_each|find)_device()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Delete redundant check (!@class) in both API class_for_each_device() and
class_find_device() with below reasons:

- The check is covered by later check (!@sp).
- Callers are unlikely to call both APIs with NULL class argument.
- Make parameter check consistent with all of other class APIs.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/class.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index e81da280af74..120d3aeb52fe 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -408,8 +408,6 @@ int class_for_each_device(const struct class *class, const struct device *start,
 	struct device *dev;
 	int error = 0;
 
-	if (!class)
-		return -EINVAL;
 	if (!sp) {
 		WARN(1, "%s called for class '%s' before it was registered",
 		     __func__, class->name);
@@ -456,8 +454,6 @@ struct device *class_find_device(const struct class *class, const struct device
 	struct class_dev_iter iter;
 	struct device *dev;
 
-	if (!class)
-		return NULL;
 	if (!sp) {
 		WARN(1, "%s called for class '%s' before it was registered",
 		     __func__, class->name);

-- 
2.34.1

Re: [PATCH 3/3] driver core: class: Delete a redundant check in APIs class_(for_each|find)_device()

[Greg Kroah-Hartman]

On Tue, Nov 05, 2024 at 08:20:24AM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Delete redundant check (!@class) in both API class_for_each_device() and
> class_find_device() with below reasons:
> 
> - The check is covered by later check (!@sp).
> - Callers are unlikely to call both APIs with NULL class argument.
> - Make parameter check consistent with all of other class APIs.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/base/class.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index e81da280af74..120d3aeb52fe 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -408,8 +408,6 @@ int class_for_each_device(const struct class *class, const struct device *start,
>  	struct device *dev;
>  	int error = 0;
>  
> -	if (!class)
> -		return -EINVAL;
>  	if (!sp) {
>  		WARN(1, "%s called for class '%s' before it was registered",
>  		     __func__, class->name);

Now, if I pass in NULL for class, I get an odd warning, AND the kernel
crashes with the dereference of class->name.

So this is not ok :(

> @@ -456,8 +454,6 @@ struct device *class_find_device(const struct class *class, const struct device
>  	struct class_dev_iter iter;
>  	struct device *dev;
>  
> -	if (!class)
> -		return NULL;
>  	if (!sp) {
>  		WARN(1, "%s called for class '%s' before it was registered",
>  		     __func__, class->name);

Same here, this change is going to break things if people get it wrong,
please leave both of these as-is.

thanks,

greg k-h

Re: [PATCH 3/3] driver core: class: Delete a redundant check in APIs class_(for_each|find)_device()

[Zijun Hu]

On 2024/11/12 19:45, Greg Kroah-Hartman wrote:
> On Tue, Nov 05, 2024 at 08:20:24AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Delete redundant check (!@class) in both API class_for_each_device() and
>> class_find_device() with below reasons:
>>
>> - The check is covered by later check (!@sp).
>> - Callers are unlikely to call both APIs with NULL class argument.
>> - Make parameter check consistent with all of other class APIs.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/base/class.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/base/class.c b/drivers/base/class.c
>> index e81da280af74..120d3aeb52fe 100644
>> --- a/drivers/base/class.c
>> +++ b/drivers/base/class.c
>> @@ -408,8 +408,6 @@ int class_for_each_device(const struct class *class, const struct device *start,
>>  	struct device *dev;
>>  	int error = 0;
>>  
>> -	if (!class)
>> -		return -EINVAL;
>>  	if (!sp) {
>>  		WARN(1, "%s called for class '%s' before it was registered",
>>  		     __func__, class->name);
> 
> Now, if I pass in NULL for class, I get an odd warning, AND the kernel
> crashes with the dereference of class->name.
> 

yes. you are right.

i did not notice "class->name" in warning message.

> So this is not ok :(
> 
>> @@ -456,8 +454,6 @@ struct device *class_find_device(const struct class *class, const struct device
>>  	struct class_dev_iter iter;
>>  	struct device *dev;
>>  
>> -	if (!class)
>> -		return NULL;
>>  	if (!sp) {
>>  		WARN(1, "%s called for class '%s' before it was registered",
>>  		     __func__, class->name);
> 
> Same here, this change is going to break things if people get it wrong,
> please leave both of these as-is.
> 

okay. agree with you.

> thanks,
> 
> greg k-h