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

Arthur Simchaev posted 1 patch 10 months ago
There is a newer version of this series
drivers/ufs/core/ufs_bsg.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH v2] ufs: core: bsg: Fix memory crash in case arpmb command failed
Posted by Arthur Simchaev 10 months ago
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
ufs_bsg_exec_advanced_rpmb_req returned error, do not set 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

Fixes: 6ff265fc5ef6 ("scsi: ufs: core: bsg: Add advanced RPMB support in ufs_bsg")
Cc: stable@vger.kernel.org
Signed-off-by: Arthur Simchaev <arthur.simchaev@sandisk.com>

---
Changes in v2:
  - Add Fixes tag
  - Elaborate commit log

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 v2] ufs: core: bsg: Fix memory crash in case arpmb command failed
Posted by Bart Van Assche 10 months ago
On 2/18/25 3:15 AM, Arthur Simchaev wrote:
> 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);
> +	}

Please make this code easier to read by changing !rpmb into rpmb and by
swapping the two sizeof() expressions.

Thanks,

Bart.
Re: [PATCH v2] ufs: core: bsg: Fix memory crash in case arpmb command failed
Posted by Bean Huo 10 months ago
On Tue, 2025-02-18 at 13:15 +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
> ufs_bsg_exec_advanced_rpmb_req returned error, do not set 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
> 
> Fixes: 6ff265fc5ef6 ("scsi: ufs: core: bsg: Add advanced RPMB support
> in ufs_bsg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arthur Simchaev <arthur.simchaev@sandisk.com>
> 
> ---
> Changes in v2:
>   - Add Fixes tag
>   - Elaborate commit log
> 
> 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,

thanks for your update. 

I tried to repoduce the issue as your steps, I didn't get this issue,
The kernel will only print this as expected: 

Err: ARPMB OP failed 0 :-22



I don't think your patch can fix your issue, becase if ufs_bsg returns 

-EINVAL(-22).  then, 


bsg_reply->result = ret(-22);

after that,  then in bsg_transport_sg_io_fn:

if (job->result < 0) {
	job->reply_len = sizeof(u32);  //overwrite the length.



Could you please provide more information how you can get this issue?
My understanding is that it is not because this job->reply_len, it is
your buffer initiated by your application?


Kind regards,
Bean