Re: [PATCH v6 2/8] scsi: ibmvfc: Add NOOP command support

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

New issues:
- [High] The `IBMVFC_NOOP` macro is incorrectly checked against `crq->valid` instead of `crq->format` in `ibmvfc_handle_scrq`.
- [High] Teardown asymmetry and Use-After-Free of `vhost->fpin_workq` during module removal.

Pre-existing issues:
- [High] Untrusted pointer dereference of `evt->queue` before validation in `ibmvfc_handle_scrq`.
--

--- Patch [2]: [PATCH v6 2/8] scsi: ibmvfc: Add NOOP command support ---
commit b1d87be5bed78116ccdb644bdfced27ffa4dfe26
Author: Dave Marquardt <[email protected]>
Subject: scsi: ibmvfc: Add NOOP command support

Add support for VFC_NOOP messages from VIOS to enable keep-alive
functionality between the client and server.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 22b4d639e74be..19a3f12615648 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -4220,9 +4230,15 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost
>  	case IBMVFC_CRQ_CMD_RSP:
>  		break;
>  	case IBMVFC_CRQ_XPORT_EVENT:
> +	case IBMVFC_NOOP:

[Severity: High]
Does this code check IBMVFC_NOOP against the correct field?

The switch statement evaluates crq->valid, but IBMVFC_NOOP is a message
format (0x03).  Will the NOOP case be functionally dead here, causing NOOP
messages to fall through to the command response handling?

>  		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 (unlikely(!evt)) {
> +		dev_err(vhost->dev, "Received null event\n");

[Severity: High]
Does this error path get triggered erroneously for every NOOP sent by the
VIOS?

If a NOOP message falls through to the command response handling, ioba is
likely zero, which would cause evt to be null.  Will this spam the kernel
log with 'Received null event' at a high rate and potentially cause disk
exhaustion?

[Severity: High]
This is a pre-existing issue, but is it safe to dereference evt->queue
before validation?

Immediately after this null check, the code executes:

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

If a malformed or malicious ioba payload is sent by the VIOS, couldn't this
cause an immediate out-of-bounds read or kernel panic when evt->queue is
dereferenced?  Should the queue pointer be securely passed down from
ibmvfc_drain_sub_crq() instead?

>  		return;
>  	}

[Severity: High]
Could the destruction of vhost->fpin_workq during module removal cause a
use-after-free?

In ibmvfc_remove():

        destroy_workqueue(vhost->fpin_workq);
        vhost->fpin_workq = NULL;

If hardware interrupts and tasklets are not disabled until
ibmvfc_release_crq_queue() is called later, an IBMVFC_AE_FPIN event could
trigger the interrupt handler and schedule the tasklet.

Would this result in ibmvfc_handle_async() executing:

        queue_work(vhost->fpin_workq, &aw->async_work_s);

on the explicitly destroyed and NULL-ed workqueue?

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