On Wed, 12 Feb 2025 at 09:08, Harshit Mogalapalli
<harshit.m.mogalapalli@xxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Eric Curtin <ecurtin@xxxxxxxxxx>
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
>
>
