lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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

Powered by Openwall GNU/*/Linux Powered by OpenVZ