Re: [PATCH] staging: comedi: Add driver register error handling in c6xdigio_attach()

[Ian Abbott]

On 27/08/2024 08:46, Aleksandr Mishin wrote:
> In c6xdigio_attach() there is a pnp_register_driver() call without return
> value check. But pnp_register_driver() can return error in some case
> (e.q. kzalloc() error in bus_add_driver() etc.).
>
> Add return value check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 2c89e159cd2f ("Staging: comedi: add c6xdigio driver")
> Signed-off-by: Aleksandr Mishin <amishin@xxxxxxxxxx>
> ---
>   drivers/comedi/drivers/c6xdigio.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/comedi/drivers/c6xdigio.c b/drivers/comedi/drivers/c6xdigio.c
> index 14b90d1c64dc..3f507f53518d 100644
> --- a/drivers/comedi/drivers/c6xdigio.c
> +++ b/drivers/comedi/drivers/c6xdigio.c
> @@ -250,7 +250,9 @@ static int c6xdigio_attach(struct comedi_device *dev,
>   		return ret;
>   
>   	/*  Make sure that PnP ports get activated */
> -	pnp_register_driver(&c6xdigio_pnp_driver);
> +	ret = pnp_register_driver(&c6xdigio_pnp_driver);
> +	if (ret)
> +		return ret;
>   
>   	s = &dev->subdevices[0];
>   	/* pwm output subdevice */

I think the problem is worse than that.  The c6xdigio_attach() function 
could be called more than once to attach multiple devices, which would 
register c6xdioio_pnp_driver more than once.  Also, if c6xdigio_attach() 
returns an error, c6xdigio_detach() will be called to clean up and 
attempt to unregister c6xdioio_pnp_driver.

What the driver is trying to do is to ensure that PNP parallel ports 
have been activated before it writes to the registers of a parallel 
port.  (Actually, all it has is a base address, but it assumes that 
belongs to one of the PNP parallel ports.)

To be honest, the driver should probably be rewritten to be a "parallel 
port device driver", but I doubt there would be many people with the 
hardware or the inclination to do this for what is really an obsolete 
bit of hardware.

Some possible ways to handle the situation:

1. Move the pnp_driver_register() / pnp_driver_unregister() stuff to be 
done at module load / unload time.

2. Remove all the PNP stuff.  The comedi_parport driver doesn't have any 
PNP stuff.  The user may have to activate the parallel port manually 
before attaching the Comedi device.

3. Remove the driver altogether.

I'd tend to favour option 2.

--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-