Re: [PATCH] scsi: fnic: Fix invalid comparison for error

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

Pre-existing issues:
- [High] vnic_dev_cmd1 accesses the dev_cmd_err array without bounds checking when the firmware returns an error code >= 5.
- [High] In vnic_dev_cmd2, raw firmware error codes are directly negated and returned instead of being mapped to standard POSIX error codes.
- [High] In the snic driver, snic_probe can return a positive error code on initialization failure, which the PCI core incorrectly treats as success.
- [High] Missing read memory barrier before reading result->error from the DMA completion ring.
--

--- Patch [1]: [PATCH] scsi: fnic: Fix invalid comparison for error ---
commit 4766be03a478e92b3d109e47a930854c49255b49
Author: Roman Demidov <[email protected]>

scsi: fnic: Fix invalid comparison for error

This patch fixes a logical flaw where an error comparison against a positive
constant always evaluated to true. It corrects the check to use the negated
error constant instead.

> diff --git a/drivers/scsi/fnic/vnic_dev.c b/drivers/scsi/fnic/vnic_dev.c
> index 991c86eb5aff7..0e5ae0540d85f 100644
> --- a/drivers/scsi/fnic/vnic_dev.c
> +++ b/drivers/scsi/fnic/vnic_dev.c
> @@ -381,7 +381,7 @@ static int vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
>  		if (result->color == color) {

[Severity: High]
This is a pre-existing issue, but is a read memory barrier needed before
accessing result->error?

When the CPU polls DMA memory for completion by checking the color flag, it
immediately reads the error field.

Without an rmb() between these two reads, could weakly ordered CPUs
speculatively read the error field before the color check is verified in
memory, potentially processing a stale error code from a previous command
completion?

>  			if (result->error) {
>  				err = -(int) result->error;

[Severity: High]
This isn't a bug introduced by this patch, but does this correctly map to
standard Linux error codes?

Unlike vnic_dev_cmd1() which translates firmware errors using the dev_cmd_err
array, vnic_dev_cmd2() negates the raw firmware error code.

For example, returning a negated ERR_EBUSY (4) results in -4 (-EINTR)
instead of -16 (-EBUSY).

Could this cause callers that check for standard POSIX errors, such as
checking for -EBUSY in fnic_nvme.c, to bypass necessary retry logic because
the error value does not match?

> -				if (err != ERR_ECMDUNKNOWN ||
> +				if (err != -ERR_ECMDUNKNOWN ||
>  						cmd != CMD_CAPABILITY)
>  					pr_err("%s:Error %d devcmd %d\n",
>  						pci_name(vdev->pdev),

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read
elsewhere in this file in vnic_dev_cmd1():

drivers/scsi/fnic/vnic_dev.c:vnic_dev_cmd1() {
        ...
        if (status & STAT_ERROR) {
                err = dev_cmd_err[(int)readq(&devcmd->args[0])];
        ...
}

If the hardware returns an unexpectedly large error code, the raw value is
cast to an integer and used as an index into the 5-element dev_cmd_err array
without any bounds validation.

Can this arbitrary hardware input cause an out-of-bounds read into unmapped
kernel memory, resulting in a panic?


[Severity: High]
This is a pre-existing issue, but I noticed similar firmware error handling
in the snic driver that may cause problems during initialization.

In _svnic_dev_cmd2(), raw firmware error codes are returned as positive
integers:

drivers/scsi/snic/vnic_dev.c:_svnic_dev_cmd2() {
        ...
        if (result->error) {
                err = (int) result->error;
                ...
                return err;
        }
        ...
}

If device initialization fails, this positive error propagates up and is
returned by snic_probe().

Since the PCI subsystem treats return values greater than zero as success,
does this cause the core device model to incorrectly bind the device even
though snic_probe() executed its error teardown path?

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