| Message ID | 20250212085853.1357906-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx() | expand |
On Wed, 12 Feb 2025 at 09:08, Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() > which is a use after free. The ->crashed function pointer is implemented > by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so > this doesn't cause a problem. But it still looks sketchy as can be. > > Fix this by moving kfree() after the last usage of bfr. > > Fixes: c559645f343a ("soc: apple: rtkit: Pass the crashlog to the crashed() callback") > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> Reviewed-by: Eric Curtin <ecurtin@redhat.com> I wish we used the attribute cleanup stuff by default more in the kernel, it would prevent a lot of things like this, it does what we want it to do, most of the time. Although I'm not sure it was introduced yet when the code was written. Is mise le meas/Regards, Eric Curtin > --- > This is based on static analysis with smatch, only compile tested. > --- > drivers/soc/apple/rtkit.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c > index 4b0783091a92..1ccec7ba640c 100644 > --- a/drivers/soc/apple/rtkit.c > +++ b/drivers/soc/apple/rtkit.c > @@ -360,7 +360,6 @@ static void apple_rtkit_crashlog_rx(struct apple_rtkit *rtk, u64 msg) > apple_rtkit_memcpy(rtk, bfr, &rtk->crashlog_buffer, 0, > rtk->crashlog_buffer.size); > apple_rtkit_crashlog_dump(rtk, bfr, rtk->crashlog_buffer.size); > - kfree(bfr); > } else { > dev_err(rtk->dev, > "RTKit: Couldn't allocate crashlog shadow buffer\n"); > @@ -369,6 +368,8 @@ static void apple_rtkit_crashlog_rx(struct apple_rtkit *rtk, u64 msg) > rtk->crashed = true; > if (rtk->ops->crashed) > rtk->ops->crashed(rtk->cookie, bfr, rtk->crashlog_buffer.size); > + > + kfree(bfr); > } > > static void apple_rtkit_ioreport_rx(struct apple_rtkit *rtk, u64 msg) > -- > 2.39.3 > >
Hi, On Wed, Feb 12, 2025, at 09:58, Harshit Mogalapalli wrote: > This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() > which is a use after free. The ->crashed function pointer is implemented > by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so > this doesn't cause a problem. But it still looks sketchy as can be. > > Fix this by moving kfree() after the last usage of bfr. > > Fixes: c559645f343a ("soc: apple: rtkit: Pass the crashlog to the > crashed() callback") This commit isn't upstream yet afaict, did you mean to post a review comment to [1] instead? [1] https://lore.kernel.org/asahi/20250202-rtkit-crashdump-v1-1-9d38615b4e12@asahilina.net/ Best, Sven
Hi, On 18/02/25 21:04, Sven Peter wrote: > Hi, > > > On Wed, Feb 12, 2025, at 09:58, Harshit Mogalapalli wrote: >> This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() >> which is a use after free. The ->crashed function pointer is implemented >> by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so >> this doesn't cause a problem. But it still looks sketchy as can be. >> >> Fix this by moving kfree() after the last usage of bfr. >> >> Fixes: c559645f343a ("soc: apple: rtkit: Pass the crashlog to the >> crashed() callback") > > This commit isn't upstream yet afaict, did you mean to post a review comment > to [1] instead? > I made the patch based on linux-next, linux-next has the patch merged. Thanks, Harshit > > [1] https://lore.kernel.org/asahi/20250202-rtkit-crashdump-v1-1-9d38615b4e12@asahilina.net/ > > > Best, > > > Sven
Hi, On Tue, Feb 18, 2025, at 17:01, Harshit Mogalapalli wrote: > Hi, > > On 18/02/25 21:04, Sven Peter wrote: >> Hi, >> >> >> On Wed, Feb 12, 2025, at 09:58, Harshit Mogalapalli wrote: >>> This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() >>> which is a use after free. The ->crashed function pointer is implemented >>> by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so >>> this doesn't cause a problem. But it still looks sketchy as can be. >>> >>> Fix this by moving kfree() after the last usage of bfr. >>> >>> Fixes: c559645f343a ("soc: apple: rtkit: Pass the crashlog to the >>> crashed() callback") >> >> This commit isn't upstream yet afaict, did you mean to post a review comment >> to [1] instead? >> > > I made the patch based on linux-next, linux-next has the patch merged. Whoops, I guess we accidentally already pushed that one to for-next without merging it. I'll merge both and adjust the commit hash in your Fixes tag. Thanks, Sven
On Wed, 12 Feb 2025 00:58:53 -0800, Harshit Mogalapalli wrote: > This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() > which is a use after free. The ->crashed function pointer is implemented > by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so > this doesn't cause a problem. But it still looks sketchy as can be. > > Fix this by moving kfree() after the last usage of bfr. > > [...] Applied, thanks! [1/1] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx() commit: 00834971f0d9e38beae37e92055b1432782827d0 Best regards,
diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c index 4b0783091a92..1ccec7ba640c 100644 --- a/drivers/soc/apple/rtkit.c +++ b/drivers/soc/apple/rtkit.c @@ -360,7 +360,6 @@ static void apple_rtkit_crashlog_rx(struct apple_rtkit *rtk, u64 msg) apple_rtkit_memcpy(rtk, bfr, &rtk->crashlog_buffer, 0, rtk->crashlog_buffer.size); apple_rtkit_crashlog_dump(rtk, bfr, rtk->crashlog_buffer.size); - kfree(bfr); } else { dev_err(rtk->dev, "RTKit: Couldn't allocate crashlog shadow buffer\n"); @@ -369,6 +368,8 @@ static void apple_rtkit_crashlog_rx(struct apple_rtkit *rtk, u64 msg) rtk->crashed = true; if (rtk->ops->crashed) rtk->ops->crashed(rtk->cookie, bfr, rtk->crashlog_buffer.size); + + kfree(bfr); } static void apple_rtkit_ioreport_rx(struct apple_rtkit *rtk, u64 msg)
This code calls kfree(bfr); and then passes "bfr" to rtk->ops->crashed() which is a use after free. The ->crashed function pointer is implemented by apple_nvme_rtkit_crashed() and it doesn't use the "bfr" pointer so this doesn't cause a problem. But it still looks sketchy as can be. Fix this by moving kfree() after the last usage of bfr. Fixes: c559645f343a ("soc: apple: rtkit: Pass the crashlog to the crashed() callback") Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> --- This is based on static analysis with smatch, only compile tested. --- drivers/soc/apple/rtkit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)