linux-kernel - Re: [PATCH v2] 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] [day] [month] [year] [list]

Message-ID: <01fd17bd-fd4e-b40a-5a1d-619a7d2c5faa@roeck-us.net>
Date:   Fri, 13 May 2022 14:37:08 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Eddie James <eajames@...ux.ibm.com>, linux-fsi@...ts.ozlabs.org
Cc:     linux-kernel@...r.kernel.org, alistair@...ple.id.au,
        joel@....id.au, jk@...abs.org
Subject: Re: [PATCH v2] fsi: occ: Prevent use after free

On 5/13/22 12:44, 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.
> Also, lock around the freeing of the device buffer and check the
> buffer before using it in the submit function.
> 
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

> ---
> Changes since v1:
>   - Add lock around freeing/nulling the buffer in occ_remove
>   - Don't bother checking the buffer in open or in write, only in submit
> 
>   drivers/fsi/fsi-occ.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fsi/fsi-occ.c b/drivers/fsi/fsi-occ.c
> index c9cc75fbdfb9..28c176d038a2 100644
> --- a/drivers/fsi/fsi-occ.c
> +++ b/drivers/fsi/fsi-occ.c
> @@ -94,6 +94,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);
> @@ -197,6 +198,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);
>   
> @@ -493,12 +495,19 @@ int fsi_occ_submit(struct device *dev, const void *request, size_t req_len,
>   	for (i = 1; i < req_len - 2; ++i)
>   		checksum += byte_request[i];
>   
> -	mutex_lock(&occ->occ_lock);
> +	rc = mutex_lock_interruptible(&occ->occ_lock);
> +	if (rc)
> +		return rc;
>   
>   	occ->client_buffer = response;
>   	occ->client_buffer_size = user_resp_len;
>   	occ->client_response_size = 0;
>   
> +	if (!occ->buffer) {
> +		rc = -ENOENT;
> +		goto done;
> +	}
> +
>   	/*
>   	 * Get a sequence number and update the counter. Avoid a sequence
>   	 * number of 0 which would pass the response check below even if the
> @@ -671,10 +680,13 @@ static int occ_remove(struct platform_device *pdev)
>   {
>   	struct occ *occ = platform_get_drvdata(pdev);
>   
> -	kvfree(occ->buffer);
> -
>   	misc_deregister(&occ->mdev);
>   
> +	mutex_lock(&occ->occ_lock);
> +	kvfree(occ->buffer);
> +	occ->buffer = NULL;
> +	mutex_unlock(&occ->occ_lock);
> +
>   	device_for_each_child(&pdev->dev, NULL, occ_unregister_child);
>   
>   	ida_simple_remove(&occ_ida, occ->idx);