linux-kernel - Re: [PATCH] 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: <1ec5693f-827d-d840-3c8e-4f2ff220cd4f@linux.ibm.com>
Date:   Fri, 13 May 2022 10:54:09 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Guenter Roeck <linux@...ck-us.net>, linux-fsi@...ts.ozlabs.org
Cc:     alistair@...ple.id.au, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fsi: occ: Prevent use after free


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@...ux.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