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@xxxxxxxxx> > > Sent: Tuesday, June 2, 2020 2:36 PM > > To: Avri Altman <Avri.Altman@xxxxxxx>; alim.akhtar@xxxxxxxxxxx; > > asutoshd@xxxxxxxxxxxxxx; jejb@xxxxxxxxxxxxx; > > martin.petersen@xxxxxxxxxx; stanley.chu@xxxxxxxxxxxx; > > beanhuo@xxxxxxxxxx; bvanassche@xxxxxxx; tomas.winkler@xxxxxxxxx; > > cang@xxxxxxxxxxxxxx > > Cc: linux-scsi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxx> > > > > --- > > > > 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