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 <[email protected]>
---
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
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 <[email protected]>
> ---
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 <[email protected]>
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);
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 <[email protected]>
>> ---
>
>
> 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 <[email protected]>
>
> 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);
On 2022/4/21 06:54, Michael Ellerman wrote:
> Hangyu Hua <[email protected]> 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/[email protected]/
rpmsg: virtio: fix possible double free in rpmsg_probe()
https://lore.kernel.org/all/[email protected]/
hwtracing: stm: fix possible double free in stm_register_device()
https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
>> ---
>> 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
On 21/04/2022 00:54, Michael Ellerman wrote:
> Hangyu Hua <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hangyu Hua <[email protected]> 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 <[email protected]>
> ---
> 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
Frederic Barrat <[email protected]> writes:
> On 21/04/2022 00:54, Michael Ellerman wrote:
>> Hangyu Hua <[email protected]> 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
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