Messages in this thread | | | Date | Fri, 13 May 2022 10:54:09 -0500 | Subject | Re: [PATCH] fsi: occ: Prevent use after free | From | Eddie James <> |
| |
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
| |