Re: [PATCH] fsi: occ: Prevent use after free

[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@xxxxxxxxxxxxx>
> > ---
> >   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