| Message ID | 20210719231127.869088-1-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: Fix memory corruption by ufshcd_read_desc_param() | expand |
> > 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); > }
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.
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
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
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
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); }
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(-)