Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <43a81bce2159ccd290e5dfe4a69199f56c379ef7.camel@gmail.com>
Subject: Re: [PATCH v4 3/5] scsi: ufs: fix potential access NULL pointer
 while memcpy
From:   Bean Huo <huobean@gmail.com>
To:     Avri Altman <Avri.Altman@wdc.com>,
        "alim.akhtar@samsung.com" <alim.akhtar@samsung.com>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "jejb@linux.ibm.com" <jejb@linux.ibm.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "stanley.chu@mediatek.com" <stanley.chu@mediatek.com>,
        "beanhuo@micron.com" <beanhuo@micron.com>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "tomas.winkler@intel.com" <tomas.winkler@intel.com>,
        "cang@codeaurora.org" <cang@codeaurora.org>
Cc:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
In-Reply-To: <SN6PR04MB4640741E1DC89A927F8A60A5FC8A0@SN6PR04MB4640.namprd04.prod.outlook.com>
References: <20200531150831.9946-1-huobean@gmail.com>
         <20200531150831.9946-4-huobean@gmail.com>
         <SN6PR04MB4640741E1DC89A927F8A60A5FC8A0@SN6PR04MB4640.namprd04.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Date:   Tue, 02 Jun 2020 13:35:53 +0200
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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