Re: [PATCH v6 03/13] scsi: fnic: Decode firmware role configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probing a device configured as an NVMe initiator causes guaranteed NULL/invalid pointer dereferences because `fnic->host` is never allocated.
--

--- Patch [3]: [PATCH v6 03/13] scsi: fnic: Decode firmware role configuration ---
commit 59ee4f0111b49617d84eeb07cca8bec4e3b8d757
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Decode firmware role configuration

This commit adds FNIC_ROLE_CONFIG_MASK to decode firmware role bits when
reading vNIC configuration. It accepts FCP and NVMe initiator roles,
reports target roles as unsupported, and logs the configured role flags.

> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 7c7f9ea5267b9..3a365ea455b1a 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
[ ... ]
> @@ -863,8 +863,27 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  					fnic->fnic_num);
>  		}
>  		break;
> +	case VFCF_FC_TARGET:
> +		dev_info(&fnic->pdev->dev,
> +			 "fnic: %d is scsi target\n",
> +			 fnic->fnic_num);
> +		err = -EOPNOTSUPP;
> +		goto err_out_fnic_role;
> +	case VFCF_FC_NVME_INITIATOR:
> +		fnic->role = FNIC_ROLE_NVME_INITIATOR;
> +		dev_info(&fnic->pdev->dev, "fnic: %d is NVME initiator\n",
> +			fnic->fnic_num);
> +		break;

[Severity: High]
If a device is configured as an NVMe initiator, this case breaks out of the
switch block without allocating fnic->host (unlike the VFCF_FC_INITIATOR
case). Could this cause a NULL pointer dereference later on?

For example, if an error occurs shortly after this in fnic_probe(), the
teardown path unconditionally calls:

err_out_fnic_set_intr_mode:
        scsi_host_put(fnic->host);

Since scsi_host_put() resolves the host pointer internally, passing NULL
results in an invalid offset pointer passed to put_device(), crashing the
kernel.

Additionally, does the success path unconditionally dereference the
unallocated host in fnic_scsi_drv_init()?

fnic_probe() -> fnic_scsi_drv_init() {
        ...
        if (fnic->config.io_throttle_count != FNIC_UCSM_DFLT_THROTTLE_CNT_BLD)
                host->can_queue = min_t(u32, FNIC_MAX_IO_REQ, ...);
        ...
}

> +	case VFCF_FC_NVME_TARGET:
> +		dev_info(&fnic->pdev->dev,
> +			 "fnic: %d is NVME target\n",
> +			 fnic->fnic_num);
> +		err = -EOPNOTSUPP;
> +		goto err_out_fnic_role;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724174811.5118-1-kartilak@cisco.com?part=3