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 NULL pointer dereference issue in
this code (CID 1519043):


> +static void ufs_lu_realize(SCSIDevice *dev, Error **errp)
> +{
> +    UfsLu *lu = DO_UPCAST(UfsLu, qdev, dev);
> +    BusState *s = qdev_get_parent_bus(&dev->qdev);
> +    UfsHc *u = UFS(s->parent);
> +    AioContext *ctx = NULL;
> +    uint64_t nb_sectors, nb_blocks;
> +
> +    if (!ufs_lu_check_constraints(lu, errp)) {
> +        return;
> +    }
> +
> +    if (lu->qdev.conf.blk) {

Here we check whether lu->qdev.conf.blk is non-NULL, implying
that it can be NULL at this point...

> +        ctx = blk_get_aio_context(lu->qdev.conf.blk);
> +        aio_context_acquire(ctx);
> +        if (!blkconf_blocksizes(&lu->qdev.conf, errp)) {
> +            goto out;
> +        }
> +    }
> +    lu->qdev.blocksize = UFS_BLOCK_SIZE;
> +    blk_get_geometry(lu->qdev.conf.blk, &nb_sectors);

...but here we pass it to blk_get_geometry(), which will
unconditionally dereference it, and crashes if it is NULL.

Either the NULL check above is unnecessary, or else this
bit of the code needs to do something else for NULL.

> +    nb_blocks = nb_sectors / (lu->qdev.blocksize / BDRV_SECTOR_SIZE);
> +    if (nb_blocks > UINT32_MAX) {
> +        nb_blocks = UINT32_MAX;
> +    }
> +    lu->qdev.max_lba = nb_blocks;
> +    lu->qdev.type = TYPE_DISK;
> +
> +    ufs_init_lu(lu);
> +    if (!ufs_add_lu(u, lu, errp)) {
> +        goto out;
> +    }
> +
> +    ufs_lu_brdv_init(lu, errp);
> +out:
> +    if (ctx) {
> +        aio_context_release(ctx);
> +    }
> +}

thanks
-- PMM