Re: [PATCH v5 2/7] scsi: ibmvfc: Add NOOP command support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Dereferencing the unvalidated, hardware-provided pointer `evt` to access `evt->queue` before validating the pointer itself, which can lead to out-of-bounds access, kernel panic, or arbitrary memory write.
--

--- Patch [2]: [PATCH v5 2/7] scsi: ibmvfc: Add NOOP command support ---
commit 62ab2a186e05dc64b296e88c19571f798767a9fa
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: Add NOOP command support

Adds support for VFC_NOOP messages from VIOS to enable keep-alive
functionality between the client and server. Defines the VFC_NOOP CRQ format
and handles it in the main and sub-CRQ handlers.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 2829f4e4b5636..04a405b0860fa 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -4212,7 +4222,15 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>  	case IBMVFC_CRQ_XPORT_EVENT:
>  		return;
>  	default:
> -		dev_err(vhost->dev, "Got and invalid message type 0x%02x\n", crq->valid);
> +		dev_err(vhost->dev, "Got an invalid message type 0x%02x\n", crq->valid);
> +		return;
> +	}
> +
> +	if (crq->format == IBMVFC_NOOP)
> +		return;
> +
> +	if (unlikely(!evt)) {
> +		dev_err(vhost->dev, "Received null event\n");
>  		return;
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but does this code dereference the
hardware-provided pointer before validating it?

Looking at ibmvfc_handle_scrq() in drivers/scsi/ibmvscsi/ibmvfc-core.c, evt is
cast directly from the untrusted crq->ioba field provided by the hypervisor:

    struct ibmvfc_event *evt = (struct ibmvfc_event *)be64_to_cpu(crq->ioba);

Although the patch adds a check for a NULL evt, it then proceeds directly to:

    if (unlikely(!ibmvfc_valid_event(&evt->queue->evt_pool, evt))) {
        ...
    }

If the hypervisor sends a corrupted or malicious non-NULL ioba value, won't
accessing evt->queue here cause an out-of-bounds access or kernel panic before
the pointer is actually validated by ibmvfc_valid_event()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2