linux-kernel - Re: [PATCH] usb: use mutex_lock in iowarrior

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024091648-excusable-unfilled-83de@gregkh>
Date: Mon, 16 Sep 2024 06:15:21 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Jeongjun Park <aha310510@...il.com>
Cc: colin.i.king@...il.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: use mutex_lock in iowarrior_read()

On Mon, Sep 16, 2024 at 01:06:29PM +0900, Jeongjun Park wrote:
> Currently, iowarrior_read() does not provide any protection for the
> iowarrior structure, so the iowarrior structure is vulnerable to data-race.
> 
> Therefore, I think it is appropriate to protect the structure using
> mutex_lock in iowarrior_read().
> 
> Fixes: 946b960d13c1 ("USB: add driver for iowarrior devices.")
> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> ---
>  drivers/usb/misc/iowarrior.c | 42 +++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
> index 6d28467ce352..7f3d37b395c3 100644
> --- a/drivers/usb/misc/iowarrior.c
> +++ b/drivers/usb/misc/iowarrior.c
> @@ -277,28 +277,41 @@ static ssize_t iowarrior_read(struct file *file, char __user *buffer,
>  	struct iowarrior *dev;
>  	int read_idx;
>  	int offset;
> +	int retval = 0;
>  
>  	dev = file->private_data;
>  
> +	if (!dev) {

How can this happen?  How was this tested?

And you didn't mention this in your changelog, why?

> +		retval = -ENODEV;
> +		goto exit;
> +	}

What prevents dev from becoming invalid after it is checked here?

> +
> +	mutex_lock(&dev->mutex);

Please use the guard() form here, it makes the change much simpler and
easier to review and maintain.

thanks,

greg k-h