* [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
@ 2021-07-19 23:11 Bart Van Assche
2021-07-20 6:45 ` Avri Altman
2021-07-29 3:37 ` Martin K. Petersen
0 siblings, 2 replies; 6+ messages in thread
From: Bart Van Assche @ 2021-07-19 23:11 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, Bart Van Assche, James E.J. Bottomley,
Can Guo, Stanley Chu, Bean Huo, Avri Altman, Asutosh Das
If param_offset > buff_len then the memcpy() statement in
ufshcd_read_desc_param() corrupts memory since it copies
256 + buff_len - param_offset bytes into a buffer with size buff_len.
Since param_offset < 256 this results in writing past the bound of the
output buffer.
Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during memcpy")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/ufs/ufshcd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 89da2cf2c969..00502ffe9b4a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3365,7 +3365,9 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
if (is_kmalloc) {
/* Make sure we don't copy more data than available */
- if (param_offset + param_size > buff_len)
+ if (buff_len < param_offset)
+ param_size = 0;
+ else if (param_offset + param_size > buff_len)
param_size = buff_len - param_offset;
memcpy(param_read_buf, &desc_buf[param_offset], param_size);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
2021-07-19 23:11 [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param() Bart Van Assche
@ 2021-07-20 6:45 ` Avri Altman
2021-07-20 16:01 ` Bart Van Assche
2021-07-29 3:37 ` Martin K. Petersen
1 sibling, 1 reply; 6+ messages in thread
From: Avri Altman @ 2021-07-20 6:45 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, James E.J. Bottomley, Can Guo,
Stanley Chu, Bean Huo, Asutosh Das
>
> If param_offset > buff_len then the memcpy() statement in
> ufshcd_read_desc_param() corrupts memory since it copies
> 256 + buff_len - param_offset bytes into a buffer with size buff_len.
> Since param_offset < 256 this results in writing past the bound of the output
> buffer.
param_offset >= buff_len is tested in line 3381?
Thanks,
Avri
>
> Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during
> memcpy")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 89da2cf2c969..00502ffe9b4a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3365,7 +3365,9 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>
> if (is_kmalloc) {
> /* Make sure we don't copy more data than available */
> - if (param_offset + param_size > buff_len)
> + if (buff_len < param_offset)
> + param_size = 0;
> + else if (param_offset + param_size > buff_len)
> param_size = buff_len - param_offset;
> memcpy(param_read_buf, &desc_buf[param_offset], param_size);
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
2021-07-20 6:45 ` Avri Altman
@ 2021-07-20 16:01 ` Bart Van Assche
0 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2021-07-20 16:01 UTC (permalink / raw)
To: Avri Altman, Martin K . Petersen
Cc: Jaegeuk Kim, linux-scsi, James E.J. Bottomley, Can Guo,
Stanley Chu, Bean Huo, Asutosh Das
On 7/19/21 11:45 PM, Avri Altman wrote:
>> If param_offset > buff_len then the memcpy() statement in
>> ufshcd_read_desc_param() corrupts memory since it copies
>> 256 + buff_len - param_offset bytes into a buffer with size buff_len.
>> Since param_offset < 256 this results in writing past the bound of the output
>> buffer.
>
> param_offset >= buff_len is tested in line 3381?
Hi Avri,
That's correct. However, a few lines lower there is the following code:
ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
desc_id, desc_index, 0,
desc_buf, &buff_len);
That call may modify (reduce) 'buff_len'. Hence, a second check is needed.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
2021-07-19 23:11 [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param() Bart Van Assche
2021-07-20 6:45 ` Avri Altman
@ 2021-07-29 3:37 ` Martin K. Petersen
2021-07-29 14:05 ` Bean Huo
1 sibling, 1 reply; 6+ messages in thread
From: Martin K. Petersen @ 2021-07-29 3:37 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, Avri Altman, Bean Huo, Can Guo, linux-scsi,
James E.J. Bottomley, Asutosh Das, Stanley Chu, Jaegeuk Kim
On Mon, 19 Jul 2021 16:11:22 -0700, Bart Van Assche wrote:
> If param_offset > buff_len then the memcpy() statement in
> ufshcd_read_desc_param() corrupts memory since it copies
> 256 + buff_len - param_offset bytes into a buffer with size buff_len.
> Since param_offset < 256 this results in writing past the bound of the
> output buffer.
Applied to 5.14/scsi-fixes, thanks!
[1/1] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
https://git.kernel.org/mkp/scsi/c/b1d5de8c6ea2
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
2021-07-29 3:37 ` Martin K. Petersen
@ 2021-07-29 14:05 ` Bean Huo
2021-07-30 2:56 ` Martin K. Petersen
0 siblings, 1 reply; 6+ messages in thread
From: Bean Huo @ 2021-07-29 14:05 UTC (permalink / raw)
To: Martin K. Petersen, Bart Van Assche
Cc: Avri Altman, Bean Huo, Can Guo, linux-scsi, James E.J. Bottomley,
Asutosh Das, Stanley Chu, Jaegeuk Kim
On Wed, 2021-07-28 at 23:37 -0400, Martin K. Petersen wrote:
> On Mon, 19 Jul 2021 16:11:22 -0700, Bart Van Assche wrote:
>
>
>
> > If param_offset > buff_len then the memcpy() statement in
> > ufshcd_read_desc_param() corrupts memory since it copies
> > 256 + buff_len - param_offset bytes into a buffer with size
> > buff_len.
> > Since param_offset < 256 this results in writing past the bound of
> > the
> > output buffer.
>
>
> Applied to 5.14/scsi-fixes, thanks!
>
>
>
> [1/1] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
>
> https://git.kernel.org/mkp/scsi/c/b1d5de8c6ea2
Hi Martin,
This patch has a problem, we should revert it.
and the correct fix patch is in Bart's another series of patch:
https://patchwork.kernel.org/project/linux-scsi/patch/20210722033439.26550-2-bvanassche@acm.org/
Bean
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param()
2021-07-29 14:05 ` Bean Huo
@ 2021-07-30 2:56 ` Martin K. Petersen
0 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2021-07-30 2:56 UTC (permalink / raw)
To: Bean Huo
Cc: Martin K. Petersen, Bart Van Assche, Avri Altman, Bean Huo,
Can Guo, linux-scsi, James E.J. Bottomley, Asutosh Das,
Stanley Chu, Jaegeuk Kim
Bean,
> This patch has a problem, we should revert it.
>
> and the correct fix patch is in Bart's another series of patch:
OK, that explains why b4 was confused. Dropped.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-30 2:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19 23:11 [PATCH] scsi: ufs: Fix memory corruption by ufshcd_read_desc_param() Bart Van Assche
2021-07-20 6:45 ` Avri Altman
2021-07-20 16:01 ` Bart Van Assche
2021-07-29 3:37 ` Martin K. Petersen
2021-07-29 14:05 ` Bean Huo
2021-07-30 2:56 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.