[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <c0654488-17b0-4ad2-bb0a-d6006d8198d6@mev.co.uk>
Date: Tue, 27 Aug 2024 11:31:32 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Aleksandr Mishin <amishin@...rgos.ru>, Greg Kroah-Hartman <gregkh@...e.de>
Cc: H Hartley Sweeten <hsweeten@...ionengravers.com>,
linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH] staging: comedi: Add driver register error handling in
c6xdigio_attach()
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@...rgos.ru>
> ---
> 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@....co.uk> || 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 )=-
Powered by blists - more mailing lists