linux-kernel - Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <f76454e3-843d-93b4-e30c-bf374d41802b@linux.ibm.com>
Date:   Thu, 21 Apr 2022 09:51:03 +0200
From:   Frederic Barrat <fbarrat@...ux.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Hangyu Hua <hbh25y@...il.com>, ajd@...ux.ibm.com,
        arnd@...db.de, gregkh@...uxfoundation.org, alastair@...ilva.org
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] misc: ocxl: fix possible double free in
 ocxl_file_register_afu



On 21/04/2022 00:54, Michael Ellerman wrote:
> Hangyu Hua <hbh25y@...il.com> writes:
>> info_release() will be called in device_unregister() when info->dev's
>> reference count is 0. So there is no need to call ocxl_afu_put() and
>> kfree() again.
> 
> Double frees are often exploitable. But it looks to me like this error
> path is not easily reachable by an attacker.
> 
> ocxl_file_register_afu() is only called from ocxl_probe(), and we only
> go to err_unregister if the sysfs or cdev initialisation fails, which
> should only happen if we hit ENOMEM, or we have a duplicate device which
> would be a device-tree/hardware error. But maybe Fred can check more
> closely, I don't know the driver that well.


The linux devices built here are based on what is parsed on the physical 
devices. Those could be FPGAs but updating the FPGA image requires root 
privilege. In any case, duplicate AFU names are possible, that's why the 
driver adds an index (the afu->config.idx part of the name) to the linux 
device name. So we would need to mess that up in the driver as well to 
have a duplicate device name.
So I would agree the double free is hard to hit.

mpe: I think this patch can be taken as is. The "beautification" I 
talked about is just that and I don't intend to work on it except if 
something else shows up.

   Fred



> cheers
> 
> 
>> Fix this by adding free_minor() and return to err_unregister error path.
>>
>> Fixes: 75ca758adbaf ("ocxl: Create a clear delineation between ocxl backend & frontend")
>> Signed-off-by: Hangyu Hua <hbh25y@...il.com>
>> ---
>>   drivers/misc/ocxl/file.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/misc/ocxl/file.c b/drivers/misc/ocxl/file.c
>> index d881f5e40ad9..6777c419a8da 100644
>> --- a/drivers/misc/ocxl/file.c
>> +++ b/drivers/misc/ocxl/file.c
>> @@ -556,7 +556,9 @@ int ocxl_file_register_afu(struct ocxl_afu *afu)
>>   
>>   err_unregister:
>>   	ocxl_sysfs_unregister_afu(info); // safe to call even if register failed
>> +	free_minor(info);
>>   	device_unregister(&info->dev);
>> +	return rc;
>>   err_put:
>>   	ocxl_afu_put(afu);
>>   	free_minor(info);
>> -- 
>> 2.25.1