| Message ID | 20220418085758.38145-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | misc: ocxl: fix possible double free in ocxl_file_register_afu | expand |
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);
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);
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
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
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
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
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
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);
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(+)