* [PATCH] fsi: occ: Prevent use after free
@ 2022-05-12 21:00 Eddie James
2022-05-12 23:10 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Eddie James @ 2022-05-12 21:00 UTC (permalink / raw)
To: linux-fsi; +Cc: linux-kernel, joel, alistair, jk, 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.
Signed-off-by: Eddie James <eajames@linux.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;
misc_deregister(&occ->mdev);
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fsi: occ: Prevent use after free
2022-05-12 21:00 [PATCH] fsi: occ: Prevent use after free Eddie James
@ 2022-05-12 23:10 ` Guenter Roeck
2022-05-13 15:54 ` Eddie James
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2022-05-12 23:10 UTC (permalink / raw)
To: Eddie James, linux-fsi; +Cc: alistair, linux-kernel
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@linux.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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fsi: occ: Prevent use after free
2022-05-12 23:10 ` Guenter Roeck
@ 2022-05-13 15:54 ` Eddie James
0 siblings, 0 replies; 3+ messages in thread
From: Eddie James @ 2022-05-13 15:54 UTC (permalink / raw)
To: Guenter Roeck, linux-fsi; +Cc: alistair, linux-kernel
On 5/12/22 18:10, Guenter Roeck wrote:
> 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@linux.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) ?
Yes, it is. And now that I think about it, there's nothing to prevent
the driver remove (and freeing buffer) while a write is ongoing.
Probably need to lock in the remove function...
Thanks
Eddie
>
> Thanks,
> Guenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-13 15:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 21:00 [PATCH] fsi: occ: Prevent use after free Eddie James
2022-05-12 23:10 ` Guenter Roeck
2022-05-13 15:54 ` Eddie James
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).