Re: [PULL 4/5] hw/ufs: Support for UFS logical unit

[Peter Maydell]

On Thu, 7 Sept 2023 at 19:18, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> From: Jeuk Kim <jeuk20.kim@samsung.com>
>
> This commit adds support for ufs logical unit.
> The LU handles processing for the SCSI command,
> unit descriptor query request.
>
> This commit enables the UFS device to process
> IO requests.
>
> Signed-off-by: Jeuk Kim <jeuk20.kim@samsung.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-id: 
> beacc504376ab6a14b1a3830bb3c69382cf6aebc.1693980783.git.jeuk20.kim@gmail.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi; Coverity points out a buffer overrun (CID 1519051) in
this commit:

> @@ -52,12 +76,18 @@ typedef struct UfsParams {
>
>  typedef struct UfsHc {
>      PCIDevice parent_obj;
> +    UfsBus bus;
>      MemoryRegion iomem;
>      UfsReg reg;
>      UfsParams params;
>      uint32_t reg_size;
>      UfsRequest *req_list;
>
> +    UfsLu *lus[UFS_MAX_LUS];

The array lus[] is UFS_MAX_LUS in size...

> +    UfsWLu *report_wlu;
> +    UfsWLu *dev_wlu;
> +    UfsWLu *boot_wlu;
> +    UfsWLu *rpmb_wlu;
>      DeviceDescriptor device_desc;
>      GeometryDescriptor geometry_desc;
>      Attributes attributes;

> @@ -716,9 +834,11 @@ static const RpmbUnitDescriptor rpmb_unit_desc = {
>
>  static QueryRespCode ufs_read_unit_desc(UfsRequest *req)
>  {
> +    UfsHc *u = req->hc;
>      uint8_t lun = req->req_upiu.qr.index;
>
> -    if (lun != UFS_UPIU_RPMB_WLUN && lun > UFS_MAX_LUS) {
> +    if (lun != UFS_UPIU_RPMB_WLUN &&
> +        (lun > UFS_MAX_LUS || u->lus[lun] == NULL)) {

...but here the guard is "> UFS_MAX_LUS", which permits
lun == UFS_MAX_LUS, which will index off the end of the array here...

>          trace_ufs_err_query_invalid_index(req->req_upiu.qr.opcode, lun);
>          return UFS_QUERY_RESULT_INVALID_INDEX;
>      }
> @@ -726,8 +846,8 @@ static QueryRespCode ufs_read_unit_desc(UfsRequest *req)
>      if (lun == UFS_UPIU_RPMB_WLUN) {
>          memcpy(&req->rsp_upiu.qr.data, &rpmb_unit_desc, 
> rpmb_unit_desc.length);
>      } else {
> -        /* unit descriptor is not yet supported */
> -        return UFS_QUERY_RESULT_INVALID_INDEX;
> +        memcpy(&req->rsp_upiu.qr.data, &u->lus[lun]->unit_desc,
> +               sizeof(u->lus[lun]->unit_desc));

...and here.

>      }
>
>      return UFS_QUERY_RESULT_SUCCESS;

Either the array needs to be larger, or the guard should be ">=".

thanks
-- PMM