[PATCH 1/2] drivers: base: Warn if driver name is not present

[PATCH 1/2] drivers: base: Warn if driver name is not present

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

If we pass a driver without a name, we end up in a NULL pointer
derefernce. Check for the name before trying to register the driver.
As we don't have a driver name to point to in the error message, we dump
the call stack to make it easier to detect the buggy driver.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/base/driver.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 57c68769e157..40fba959c140 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -149,6 +149,12 @@ int driver_register(struct device_driver *drv)
 	int ret;
 	struct device_driver *other;
 
+	if (!drv->name) {
+		pr_err("Driver has no name.\n");
+		dump_stack();
+		return -EINVAL;
+	}
+
 	if (!drv->bus->p) {
 		pr_err("Driver '%s' was unable to register with bus_type '%s' because the bus was not initialized.\n",
 			   drv->name, drv->bus->name);
-- 
2.26.2

[PATCH 2/2] drivers: base: Convert to printk alias functions

[matthias.bgg]

From: Matthias Brugger <mbrugger@suse.com>

The file mixes printk calls together with calls to pr_*().
Covert to printk alias functions to unify the code.

Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
 drivers/base/driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 40fba959c140..aa3b3a226a02 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -164,12 +164,12 @@ int driver_register(struct device_driver *drv)
 	if ((drv->bus->probe && drv->probe) ||
 	    (drv->bus->remove && drv->remove) ||
 	    (drv->bus->shutdown && drv->shutdown))
-		printk(KERN_WARNING "Driver '%s' needs updating - please use "
+		pr_warn("Driver '%s' needs updating - please use "
 			"bus_type methods\n", drv->name);
 
 	other = driver_find(drv->name, drv->bus);
 	if (other) {
-		printk(KERN_ERR "Error: Driver '%s' is already registered, "
+		pr_err("Error: Driver '%s' is already registered, "
 			"aborting...\n", drv->name);
 		return -EBUSY;
 	}
-- 
2.26.2

Re: [PATCH 1/2] drivers: base: Warn if driver name is not present

[Greg KH]

On Mon, Jun 08, 2020 at 11:52:16AM +0200, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <mbrugger@suse.com>
> 
> If we pass a driver without a name, we end up in a NULL pointer
> derefernce.

That's a very good reason not to have a driver without a name :)

What in-kernel driver does this?

> Check for the name before trying to register the driver.
> As we don't have a driver name to point to in the error message, we dump
> the call stack to make it easier to detect the buggy driver.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> ---
>  drivers/base/driver.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 57c68769e157..40fba959c140 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -149,6 +149,12 @@ int driver_register(struct device_driver *drv)
>  	int ret;
>  	struct device_driver *other;
>  
> +	if (!drv->name) {
> +		pr_err("Driver has no name.\n");
> +		dump_stack();
> +		return -EINVAL;

Ick, no, an oops-traceback for doing something dumb like this should be
all that we need, right?

How "hardened" do we need to make internal apis anyway?  What's the odds
that if this does trigger, the driver author would even notice it?

thanks,

greg k-h

Re: [PATCH 1/2] drivers: base: Warn if driver name is not present

[Matthias Brugger]

On 08/06/2020 12:57, Greg KH wrote:
> On Mon, Jun 08, 2020 at 11:52:16AM +0200, matthias.bgg@kernel.org wrote:
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> If we pass a driver without a name, we end up in a NULL pointer
>> derefernce.
> 
> That's a very good reason not to have a driver without a name :)
> 
> What in-kernel driver does this?
> 
>> Check for the name before trying to register the driver.
>> As we don't have a driver name to point to in the error message, we dump
>> the call stack to make it easier to detect the buggy driver.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>> ---
>>  drivers/base/driver.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
>> index 57c68769e157..40fba959c140 100644
>> --- a/drivers/base/driver.c
>> +++ b/drivers/base/driver.c
>> @@ -149,6 +149,12 @@ int driver_register(struct device_driver *drv)
>>  	int ret;
>>  	struct device_driver *other;
>>  
>> +	if (!drv->name) {
>> +		pr_err("Driver has no name.\n");
>> +		dump_stack();
>> +		return -EINVAL;
> 
> Ick, no, an oops-traceback for doing something dumb like this should be
> all that we need, right?
> 
> How "hardened" do we need to make internal apis anyway?  What's the odds
> that if this does trigger, the driver author would even notice it?
> 

We just had the case that a driver got accepted in a maintainer repository
without a name. Which got later found by the kernel test robot.

I agree with you that it probably doesn't make much sense to check for this kind
of bugs, as it should be discoverable if you test your code, before you submit.

I propose to ignore this patch.

Regards,
Matthias

Re: [PATCH 1/2] drivers: base: Warn if driver name is not present

[Greg KH]

On Mon, Jun 08, 2020 at 01:48:28PM +0200, Matthias Brugger wrote:
> 
> 
> On 08/06/2020 12:57, Greg KH wrote:
> > On Mon, Jun 08, 2020 at 11:52:16AM +0200, matthias.bgg@kernel.org wrote:
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> If we pass a driver without a name, we end up in a NULL pointer
> >> derefernce.
> > 
> > That's a very good reason not to have a driver without a name :)
> > 
> > What in-kernel driver does this?
> > 
> >> Check for the name before trying to register the driver.
> >> As we don't have a driver name to point to in the error message, we dump
> >> the call stack to make it easier to detect the buggy driver.
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >> ---
> >>  drivers/base/driver.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> >> index 57c68769e157..40fba959c140 100644
> >> --- a/drivers/base/driver.c
> >> +++ b/drivers/base/driver.c
> >> @@ -149,6 +149,12 @@ int driver_register(struct device_driver *drv)
> >>  	int ret;
> >>  	struct device_driver *other;
> >>  
> >> +	if (!drv->name) {
> >> +		pr_err("Driver has no name.\n");
> >> +		dump_stack();
> >> +		return -EINVAL;
> > 
> > Ick, no, an oops-traceback for doing something dumb like this should be
> > all that we need, right?
> > 
> > How "hardened" do we need to make internal apis anyway?  What's the odds
> > that if this does trigger, the driver author would even notice it?
> > 
> 
> We just had the case that a driver got accepted in a maintainer repository
> without a name. Which got later found by the kernel test robot.

That driver had obviously never actually been run before :(

> I agree with you that it probably doesn't make much sense to check for this kind
> of bugs, as it should be discoverable if you test your code, before you submit.
> 
> I propose to ignore this patch.

Thanks, now dropped!

greg k-h