linux-kernel - Re: [PATCH] fsi: occ: Prevent use after free

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 May 2022 16:10:46 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Eddie James <eajames@...ux.ibm.com>, linux-fsi@...ts.ozlabs.org
Cc:     alistair@...ple.id.au, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fsi: occ: Prevent use after free

On 5/12/22 14:00, Eddie James wrote:
> Use get_device and put_device in the open and close functions to
> make sure the device doesn't get freed while a file descriptor is
> open.
> 
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
> ---
>   drivers/fsi/fsi-occ.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index c9cc75fbdfb9..9e48dc62b1c5 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -82,6 +82,9 @@ static int occ_open(struct inode *inode, struct file *file)
>   	struct miscdevice *mdev = file->private_data;
>   	struct occ *occ = to_occ(mdev);
>   
> +	if (!occ->buffer)
> +		return -ENOENT;
> +
>   	if (!client)
>   		return -ENOMEM;
>   
> @@ -94,6 +97,7 @@ static int occ_open(struct inode *inode, struct file *file)
>   	client->occ = occ;
>   	mutex_init(&client->lock);
>   	file->private_data = client;
> +	get_device(occ->dev);
>   
>   	/* We allocate a 1-page buffer, make sure it all fits */
>   	BUILD_BUG_ON((OCC_CMD_DATA_BYTES + 3) > PAGE_SIZE);
> @@ -143,7 +147,7 @@ static ssize_t occ_write(struct file *file, const char __user *buf,
>   	ssize_t rc;
>   	u8 *cmd;
>   
> -	if (!client)
> +	if (!client || !client->occ->buffer)
>   		return -ENODEV;
>   
>   	if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
> @@ -197,6 +201,7 @@ static int occ_release(struct inode *inode, struct file *file)
>   {
>   	struct occ_client *client = file->private_data;
>   
> +	put_device(client->occ->dev);
>   	free_page((unsigned long)client->buffer);
>   	kfree(client);
>   
> @@ -672,6 +677,7 @@ static int occ_remove(struct platform_device *pdev)
>   	struct occ *occ = platform_get_drvdata(pdev);
>   
>   	kvfree(occ->buffer);
> +	occ->buffer = NULL;

Isn't this slightly racy (there is no guarantee that occ->buffer is updated
by the time it is used by the write function, and there is no synchronization
across CPUs which ensures that the pointer is actually written to memory
before it is used) ?

Thanks,
Guenter