[PATCH linux dev-5.15] fsi: occ: Prevent use after free

[PATCH linux dev-5.15] fsi: occ: Prevent use after free

[Eddie James]

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@linux.ibm.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 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 3d04e8baecbb..8f7f602b909d 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
@@ -674,10 +683,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);
-- 
2.27.0

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

[Joel Stanley]

On Wed, 18 May 2022 at 13:49, Eddie James <eajames@linux.ibm.com> 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@linux.ibm.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Applied, thanks.

> ---
>  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 3d04e8baecbb..8f7f602b909d 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
> @@ -674,10 +683,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);
> --
> 2.27.0
>