All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.