[PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

[PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

[Arthur Simchaev]

In case the device doesn't support arpmb, the kernel get memory crash
due to copy user data in bsg_transport_sg_io_fn level. So in case
ufshcd_send_bsg_uic_cmd returned error, do not change the job's reply_len.

Memory crash backtrace:
3,1290,531166405,-;ufshcd 0000:00:12.5: ARPMB OP failed: error code -22

4,1308,531166555,-;Call Trace:

4,1309,531166559,-; <TASK>

4,1310,531166565,-; ? show_regs+0x6d/0x80

4,1311,531166575,-; ? die+0x37/0xa0

4,1312,531166583,-; ? do_trap+0xd4/0xf0

4,1313,531166593,-; ? do_error_trap+0x71/0xb0

4,1314,531166601,-; ? usercopy_abort+0x6c/0x80

4,1315,531166610,-; ? exc_invalid_op+0x52/0x80

4,1316,531166622,-; ? usercopy_abort+0x6c/0x80

4,1317,531166630,-; ? asm_exc_invalid_op+0x1b/0x20

4,1318,531166643,-; ? usercopy_abort+0x6c/0x80

4,1319,531166652,-; __check_heap_object+0xe3/0x120

4,1320,531166661,-; check_heap_object+0x185/0x1d0

4,1321,531166670,-; __check_object_size.part.0+0x72/0x150

4,1322,531166679,-; __check_object_size+0x23/0x30

4,1323,531166688,-; bsg_transport_sg_io_fn+0x314/0x3b0

Signed-off-by: Arthur Simchaev <arthur.simchaev@sandisk.com>
---
 drivers/ufs/core/ufs_bsg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
index 8d4ad0a3f2cf..a8ed9bc6e4f1 100644
--- a/drivers/ufs/core/ufs_bsg.c
+++ b/drivers/ufs/core/ufs_bsg.c
@@ -194,10 +194,12 @@ static int ufs_bsg_request(struct bsg_job *job)
 	ufshcd_rpm_put_sync(hba);
 	kfree(buff);
 	bsg_reply->result = ret;
-	job->reply_len = !rpmb ? sizeof(struct ufs_bsg_reply) : sizeof(struct ufs_rpmb_reply);
 	/* complete the job here only if no error */
-	if (ret == 0)
+	if (ret == 0) {
+		job->reply_len = !rpmb ? sizeof(struct ufs_bsg_reply) :
+					 sizeof(struct ufs_rpmb_reply);
 		bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
+	}
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

[Bean Huo]

On Mon, 2025-02-17 at 18:43 +0200, Arthur Simchaev wrote:
> In case the device doesn't support arpmb, the kernel get memory crash
> due to copy user data in bsg_transport_sg_io_fn level. So in case
> ufshcd_send_bsg_uic_cmd returned error, do not change the job's
> reply_len.
> 
> Memory crash backtrace:
> 3,1290,531166405,-;ufshcd 0000:00:12.5: ARPMB OP failed: error code -
> 22
> 
> 4,1308,531166555,-;Call Trace:
> 
> 4,1309,531166559,-; <TASK>
> 
> 4,1310,531166565,-; ? show_regs+0x6d/0x80
> 
> 4,1311,531166575,-; ? die+0x37/0xa0
> 
> 4,1312,531166583,-; ? do_trap+0xd4/0xf0
> 
> 4,1313,531166593,-; ? do_error_trap+0x71/0xb0
> 
> 4,1314,531166601,-; ? usercopy_abort+0x6c/0x80
> 
> 4,1315,531166610,-; ? exc_invalid_op+0x52/0x80
> 
> 4,1316,531166622,-; ? usercopy_abort+0x6c/0x80
> 
> 4,1317,531166630,-; ? asm_exc_invalid_op+0x1b/0x20
> 
> 4,1318,531166643,-; ? usercopy_abort+0x6c/0x80
> 
> 4,1319,531166652,-; __check_heap_object+0xe3/0x120
> 
> 4,1320,531166661,-; check_heap_object+0x185/0x1d0
> 
> 4,1321,531166670,-; __check_object_size.part.0+0x72/0x150
> 
> 4,1322,531166679,-; __check_object_size+0x23/0x30
> 
> 4,1323,531166688,-; bsg_transport_sg_io_fn+0x314/0x3b0
> 
> Signed-off-by: Arthur Simchaev <arthur.simchaev@sandisk.com>
> ---
>  drivers/ufs/core/ufs_bsg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs_bsg.c b/drivers/ufs/core/ufs_bsg.c
> index 8d4ad0a3f2cf..a8ed9bc6e4f1 100644
> --- a/drivers/ufs/core/ufs_bsg.c
> +++ b/drivers/ufs/core/ufs_bsg.c
> @@ -194,10 +194,12 @@ static int ufs_bsg_request(struct bsg_job *job)
>         ufshcd_rpm_put_sync(hba);
>         kfree(buff);
>         bsg_reply->result = ret;
> -       job->reply_len = !rpmb ? sizeof(struct ufs_bsg_reply) :
> sizeof(struct ufs_rpmb_reply);
>         /* complete the job here only if no error */
> -       if (ret == 0)
> +       if (ret == 0) {
> +               job->reply_len = !rpmb ? sizeof(struct ufs_bsg_reply)
> :
> +                                        sizeof(struct
> ufs_rpmb_reply);
>                 bsg_job_done(job, ret, bsg_reply-
> >reply_payload_rcv_len);
> +       }
>  
>         return ret;
>  }


Arthur, 

The change appears logical because we only need to copy the payload
when the operation is successful. 

However, I don't fully understand how the memory crash could occur. If
the function in question is `ufshcd_send_bsg_uic_cmd`, it wouldn't
involve RPMB access, meaning `rpmb` would be `false`. In that case, the
size used would be `sizeof(struct ufs_bsg_reply)`, which has no
connection to the advanced RPMB functionality. 

Kind regards,
Bean

Re: [PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

[Bean Huo]

On Mon, 2025-02-17 at 18:43 +0200, Arthur Simchaev wrote:
> In case the device doesn't support arpmb, the kernel get memory crash
> due to copy user data in bsg_transport_sg_io_fn level. So in case
> ufshcd_send_bsg_uic_cmd returned error, do not change the job's
> reply_len.
> 
> Memory crash backtrace:
> 3,1290,531166405,-;ufshcd 0000:00:12.5: ARPMB OP failed: error code -
> 22


It is Advanced RPMB access and not related to the UIC command, 

If the deivce didn't support advanced rpmb, got return -EINVAL(-22). 

In this case, in bsg_transport_sg_io_fn, 

if (job->result < 0) {
	job->reply_len = sizeof(u32); 

then:

 int len = min(hdr->max_response_len, job->reply_len); 
	if (copy_to_user(uptr64(hdr->response), job->reply, len))


It looks like you didn't initialize the correct response buffer from
user space.

Could you rephrase your commit message, add a Fixes tag, and resubmit?


Kind regards,
Bean

RE: [PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

[Arthur Simchaev]

Done

-----Original Message-----
From: Bean Huo <huobean@gmail.com> 
Sent: Monday, February 17, 2025 8:40 PM
To: Arthur Simchaev <Arthur.Simchaev@sandisk.com>; martin.petersen@oracle.com
Cc: Avri Altman <Avri.Altman@sandisk.com>; Avi Shchislowski <Avi.Shchislowski@sandisk.com>; beanhuo@micron.com; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; bvanassche@acm.org
Subject: Re: [PATCH] scsi: ufs: core: Fix memory crash in case arpmb command failed

On Mon, 2025-02-17 at 18:43 +0200, Arthur Simchaev wrote:
> In case the device doesn't support arpmb, the kernel get memory crash 
> due to copy user data in bsg_transport_sg_io_fn level. So in case 
> ufshcd_send_bsg_uic_cmd returned error, do not change the job's 
> reply_len.
> 
> Memory crash backtrace:
> 3,1290,531166405,-;ufshcd 0000:00:12.5: ARPMB OP failed: error code -
> 22


It is Advanced RPMB access and not related to the UIC command, 

If the deivce didn't support advanced rpmb, got return -EINVAL(-22). 

In this case, in bsg_transport_sg_io_fn, 

if (job->result < 0) {
	job->reply_len = sizeof(u32); 

then:

 int len = min(hdr->max_response_len, job->reply_len); 
	if (copy_to_user(uptr64(hdr->response), job->reply, len))


It looks like you didn't initialize the correct response buffer from user space.

Could you rephrase your commit message, add a Fixes tag, and resubmit?


Kind regards,
Bean