[PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Hangyu Hua]

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.

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

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Frederic Barrat]

On 18/04/2022 10:57, Hangyu Hua wrote:
> 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.
> 
> 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@gmail.com>
> ---


Thanks for fixing that error path!
I'm now thinking it would be cleaner to have the call to free_minor() in 
the info_release() callback but that would be another patch.
In any case:
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>

   Fred


>   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);

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Hangyu Hua]

On 2022/4/19 17:09, Frederic Barrat wrote:
> 
> 
> On 18/04/2022 10:57, Hangyu Hua wrote:
>> 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.
>>
>> 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@gmail.com>
>> ---
> 
> 
> Thanks for fixing that error path!
> I'm now thinking it would be cleaner to have the call to free_minor() in 
> the info_release() callback but that would be another patch.
> In any case:
> Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> 
>    Fred
> 

I think it is a good idea to use callbacks to handle all garbage 
collections. And free_minor is used only in ocxl_file_register_afu() 
andocxl_file_unregister_afu(). So this should only require minor changes.

Thanks.

> 
>>   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);

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Michael Ellerman]

Hangyu Hua <hbh25y@gmail.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.

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

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Hangyu Hua]

On 2022/4/21 06:54, Michael Ellerman wrote:
> Hangyu Hua <hbh25y@gmail.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.
> 
> cheers
> 

Hi Michael,

You are right. It is hard to exploit at least in my understanding. 
That's why I didn't give this to security@. But it still need to be fix 
out. By the way, if you are interesting in this kind of bug, you can 
check out the other three patches I submitted recently.

rpmsg: virtio: fix possible double free in rpmsg_virtio_add_ctrl_dev()
https://lore.kernel.org/all/20220418101724.42174-1-hbh25y@gmail.com/

rpmsg: virtio: fix possible double free in rpmsg_probe()
https://lore.kernel.org/all/20220418093144.40859-1-hbh25y@gmail.com/

hwtracing: stm: fix possible double free in stm_register_device()
https://lore.kernel.org/all/20220418081632.35121-1-hbh25y@gmail.com/

They are all the similar bugs i could find in the kernel.

Thanks.

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

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Frederic Barrat]

On 21/04/2022 00:54, Michael Ellerman wrote:
> Hangyu Hua <hbh25y@gmail.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@gmail.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

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Michael Ellerman]

Frederic Barrat <fbarrat@linux.ibm.com> writes:
> On 21/04/2022 00:54, Michael Ellerman wrote:
>> Hangyu Hua <hbh25y@gmail.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.

Thanks for confirming.

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

OK, will pick this up.

cheers

Re: [PATCH] misc: ocxl: fix possible double free in ocxl_file_register_afu

[Michael Ellerman]

On Mon, 18 Apr 2022 16:57:58 +0800, Hangyu Hua wrote:
> 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.
> 
> Fix this by adding free_minor() and return to err_unregister error path.
> 
> 
> [...]

Applied to powerpc/next.

[1/1] misc: ocxl: fix possible double free in ocxl_file_register_afu
      https://git.kernel.org/powerpc/c/950cf957fe34d40d63dfa3bf3968210430b6491e

cheers