[PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Harshit Mogalapalli]

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

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

Re: [PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Eric Curtin]

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

Re: [PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Sven Peter]

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

Re: [PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Harshit Mogalapalli]

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

Re: [PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Sven Peter]

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

Re: [PATCH] soc: apple: rtkit: Fix use-after-free in apple_rtkit_crashlog_rx()

[Sven Peter]

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,
-- 
Sven Peter <sven@svenpeter.dev>