| Message ID | 20250217164330.245612-1-arthur.simchaev@sandisk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: core: Fix memory crash in case arpmb command failed | expand |
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
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
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
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; }
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(-)