diff mbox series

[v4,3/5] scsi: ufs: fix potential access NULL pointer while memcpy

Message ID 20200531150831.9946-4-huobean@gmail.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: cleanup ufs initialization | expand

Commit Message

Bean Huo May 31, 2020, 3:08 p.m. UTC 
From: Bean Huo <beanhuo@micron.com>

If param_offset is not 0, the memcpy length shouldn't be the
true descriptor length.

Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out ufshcd_read_desc_param")
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Avri Altman June 1, 2020, 6:25 a.m. UTC | #1 
Hi,

> If param_offset is not 0, the memcpy length shouldn't be the
> true descriptor length.
> 
> Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out ufshcd_read_desc_param")
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f7e8bfefe3d4..bc52a0e89cd3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
> 
>         /* Check wherher we will not copy more data, than available */
>         if (is_kmalloc && param_size > buff_len)
> -               param_size = buff_len;
> +               param_size = buff_len - param_offset;
But Is_kmalloc is true if (param_offset != 0 || param_size < buff_len)
So  if (is_kmalloc && param_size > buff_len) implies that param_offset is 0,
Or did I get it wrong?

Still, I think that there is a problem here because nowhere we are checking that  
param_offset + param_size < buff_len, which now can happen because of ufs-bsg.

Maybe you can add it and get rid of that is_kmalloc which is an awkward way to test for valid values?

Thanks,
Avri
> 
>         if (is_kmalloc)
>                 memcpy(param_read_buf, &desc_buf[param_offset], param_size);
> --
> 2.17.1
Bean Huo June 2, 2020, 11:35 a.m. UTC | #2 
hi Avri
thanks review.


On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> Hi,
> 
> > If param_offset is not 0, the memcpy length shouldn't be the
> > true descriptor length.
> > 
> > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > ufshcd_read_desc_param")
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > *hba,
> > 
> >         /* Check wherher we will not copy more data, than available
> > */
> >         if (is_kmalloc && param_size > buff_len)
> > -               param_size = buff_len;
> > +               param_size = buff_len - param_offset;
> 
> But Is_kmalloc is true if (param_offset != 0 || param_size <
> buff_len)
> So  if (is_kmalloc && param_size > buff_len) implies that
> param_offset is 0,
> Or did I get it wrong?

If param_offset is 0, This willn't get any wrong, after this patch, it
is the same since offset is 0. As mentioned in the commit message, this
patch is only for the case of param_offset is not 0.

> 
> Still, I think that there is a problem here because nowhere we are
> checking that  
> param_offset + param_size < buff_len, which now can happen because of
> ufs-bsg.
> Maybe you can add it and get rid of that is_kmalloc which is an
> awkward way to test for valid values?

let me explain further:
we have these conditinos:

1) param_offset == 0, param_size >= buff_len;//no problem, 
ufshcd_query_descriptor_retry() will read descripor with true
descriptor length, and no memcpy() called.


2) param_offset == 0, param_size < buff_len;// no problem, 
ufshcd_query_descriptor_retry() will read descripor with true
descriptor length buff_len, and memcpy() with param_size length.


3) param_offset != 0, param_offset + param_size <= buff_len;// no
problem, ufshcd_query_descriptor_retry() will read descripor with true
descriptor length, and memcpy() with param_size length.


4) param_offset != 0, param_offset + param_size > buff_len;// NULL
pointer reference problem, since ufshcd_query_descriptor_retry() will
read descripor with true descriptor length, and memcpy() with buff_len
length. correct memcpy length should be (buff_len - param_offset)

param_offset + param_size < buff_len doesn't need to add, and
is_kmalloc is very hard to be removed based on current flow.

so, the correct fixup patch shoulbe be like this:


-if (is_kmalloc && param_size > buff_len)
-	param_size = buff_len
+if (is_kmalloc && (param_size + param_offset) > buff_len) 
+	param_size = buff_len - param_offset;


how do you think about it? if no problem, I will update it in next
version patch.

thanks,

Bean
Avri Altman June 2, 2020, 12:25 p.m. UTC | #3 
How about something like the untested attached?

Thanks,
Avri

> -----Original Message-----
> From: Bean Huo <huobean@gmail.com>
> Sent: Tuesday, June 2, 2020 2:36 PM
> To: Avri Altman <Avri.Altman@wdc.com>; alim.akhtar@samsung.com;
> asutoshd@codeaurora.org; jejb@linux.ibm.com;
> martin.petersen@oracle.com; stanley.chu@mediatek.com;
> beanhuo@micron.com; bvanassche@acm.org; tomas.winkler@intel.com;
> cang@codeaurora.org
> Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while
> memcpy
> 
> CAUTION: This email originated from outside of Western Digital. Do not click
> on links or open attachments unless you recognize the sender and know that
> the content is safe.
> 
> 
> hi Avri
> thanks review.
> 
> 
> On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> > Hi,
> >
> > > If param_offset is not 0, the memcpy length shouldn't be the
> > > true descriptor length.
> > >
> > > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > > ufshcd_read_desc_param")
> > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > > ---
> > >  drivers/scsi/ufs/ufshcd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > > *hba,
> > >
> > >         /* Check wherher we will not copy more data, than available
> > > */
> > >         if (is_kmalloc && param_size > buff_len)
> > > -               param_size = buff_len;
> > > +               param_size = buff_len - param_offset;
> >
> > But Is_kmalloc is true if (param_offset != 0 || param_size <
> > buff_len)
> > So  if (is_kmalloc && param_size > buff_len) implies that
> > param_offset is 0,
> > Or did I get it wrong?
> 
> If param_offset is 0, This willn't get any wrong, after this patch, it
> is the same since offset is 0. As mentioned in the commit message, this
> patch is only for the case of param_offset is not 0.
> 
> >
> > Still, I think that there is a problem here because nowhere we are
> > checking that
> > param_offset + param_size < buff_len, which now can happen because of
> > ufs-bsg.
> > Maybe you can add it and get rid of that is_kmalloc which is an
> > awkward way to test for valid values?
> 
> let me explain further:
> we have these conditinos:
> 
> 1) param_offset == 0, param_size >= buff_len;//no problem,
> ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length, and no memcpy() called.
> 
> 
> 2) param_offset == 0, param_size < buff_len;// no problem,
> ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length buff_len, and memcpy() with param_size length.
> 
> 
> 3) param_offset != 0, param_offset + param_size <= buff_len;// no
> problem, ufshcd_query_descriptor_retry() will read descripor with true
> descriptor length, and memcpy() with param_size length.
> 
> 
> 4) param_offset != 0, param_offset + param_size > buff_len;// NULL
> pointer reference problem, since ufshcd_query_descriptor_retry() will
> read descripor with true descriptor length, and memcpy() with buff_len
> length. correct memcpy length should be (buff_len - param_offset)
> 
> param_offset + param_size < buff_len doesn't need to add, and
> is_kmalloc is very hard to be removed based on current flow.
> 
> so, the correct fixup patch shoulbe be like this:
> 
> 
> -if (is_kmalloc && param_size > buff_len)
> -       param_size = buff_len
> +if (is_kmalloc && (param_size + param_offset) > buff_len)
> +       param_size = buff_len - param_offset;
> 
> 
> how do you think about it? if no problem, I will update it in next
> version patch.
> 
> thanks,
> 
> Bean
Avri Altman June 2, 2020, 1:38 p.m. UTC | #4 
But this is just a suggestion.
Your way is fine too.

Thanks,
Avri

> 
> How about something like the untested attached?
> 
> Thanks,
> Avri
> 
> > -----Original Message-----
> > From: Bean Huo <huobean@gmail.com>
> > Sent: Tuesday, June 2, 2020 2:36 PM
> > To: Avri Altman <Avri.Altman@wdc.com>; alim.akhtar@samsung.com;
> > asutoshd@codeaurora.org; jejb@linux.ibm.com;
> > martin.petersen@oracle.com; stanley.chu@mediatek.com;
> > beanhuo@micron.com; bvanassche@acm.org; tomas.winkler@intel.com;
> > cang@codeaurora.org
> > Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer while
> > memcpy
> >
> > CAUTION: This email originated from outside of Western Digital. Do not click
> > on links or open attachments unless you recognize the sender and know
> that
> > the content is safe.
> >
> >
> > hi Avri
> > thanks review.
> >
> >
> > On Mon, 2020-06-01 at 06:25 +0000, Avri Altman wrote:
> > > Hi,
> > >
> > > > If param_offset is not 0, the memcpy length shouldn't be the
> > > > true descriptor length.
> > > >
> > > > Fixes: a4b0e8a4e92b ("scsi: ufs: Factor out
> > > > ufshcd_read_desc_param")
> > > > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index f7e8bfefe3d4..bc52a0e89cd3 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -3211,7 +3211,7 @@ int ufshcd_read_desc_param(struct ufs_hba
> > > > *hba,
> > > >
> > > >         /* Check wherher we will not copy more data, than available
> > > > */
> > > >         if (is_kmalloc && param_size > buff_len)
> > > > -               param_size = buff_len;
> > > > +               param_size = buff_len - param_offset;
> > >
> > > But Is_kmalloc is true if (param_offset != 0 || param_size <
> > > buff_len)
> > > So  if (is_kmalloc && param_size > buff_len) implies that
> > > param_offset is 0,
> > > Or did I get it wrong?
> >
> > If param_offset is 0, This willn't get any wrong, after this patch, it
> > is the same since offset is 0. As mentioned in the commit message, this
> > patch is only for the case of param_offset is not 0.
> >
> > >
> > > Still, I think that there is a problem here because nowhere we are
> > > checking that
> > > param_offset + param_size < buff_len, which now can happen because of
> > > ufs-bsg.
> > > Maybe you can add it and get rid of that is_kmalloc which is an
> > > awkward way to test for valid values?
> >
> > let me explain further:
> > we have these conditinos:
> >
> > 1) param_offset == 0, param_size >= buff_len;//no problem,
> > ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length, and no memcpy() called.
> >
> >
> > 2) param_offset == 0, param_size < buff_len;// no problem,
> > ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length buff_len, and memcpy() with param_size length.
> >
> >
> > 3) param_offset != 0, param_offset + param_size <= buff_len;// no
> > problem, ufshcd_query_descriptor_retry() will read descripor with true
> > descriptor length, and memcpy() with param_size length.
> >
> >
> > 4) param_offset != 0, param_offset + param_size > buff_len;// NULL
> > pointer reference problem, since ufshcd_query_descriptor_retry() will
> > read descripor with true descriptor length, and memcpy() with buff_len
> > length. correct memcpy length should be (buff_len - param_offset)
> >
> > param_offset + param_size < buff_len doesn't need to add, and
> > is_kmalloc is very hard to be removed based on current flow.
> >
> > so, the correct fixup patch shoulbe be like this:
> >
> >
> > -if (is_kmalloc && param_size > buff_len)
> > -       param_size = buff_len
> > +if (is_kmalloc && (param_size + param_offset) > buff_len)
> > +       param_size = buff_len - param_offset;
> >
> >
> > how do you think about it? if no problem, I will update it in next
> > version patch.
> >
> > thanks,
> >
> > Bean
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f7e8bfefe3d4..bc52a0e89cd3 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3211,7 +3211,7 @@  int ufshcd_read_desc_param(struct ufs_hba *hba,
 
 	/* Check wherher we will not copy more data, than available */
 	if (is_kmalloc && param_size > buff_len)
-		param_size = buff_len;
+		param_size = buff_len - param_offset;
 
 	if (is_kmalloc)
 		memcpy(param_read_buf, &desc_buf[param_offset], param_size);