Re: [PATCH] scsi: qedi: Initialize callback state before registration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Network packet reception in `qedi_ll2_rx` lacks bounds checks, allowing malformed packets to cause out-of-bounds reads or NULL pointer dereferences.
- [High] Race condition and missing error checks for `kthread_run()` return value lead to invalid pointer dereferences during LL2 Rx processing or module teardown.
- [High] The patch leaves `recovery_work` and `board_disable_work` uninitialized when registering `qedi_cb_ops` callbacks, exposing uninitialized work structures to asynchronous hardware events.
- [High] User-configured custom MTU settings are silently overwritten to defaults during hardware recovery.
- [High] `iscsi_host_add` exposes the device to userspace before crucial connection structures (`ep_tbl`) are allocated, creating a race condition.
- [High] The background LL2 receiver kthread and hardware subsystem are not stopped on the `__qedi_probe` error path, leading to Use-After-Free.
- [High] The error path for `iscsi_host_add` failure calls `iscsi_host_remove`, which will crash when attempting to remove an un-added host.
--

commit 7badf30e1ff5d4629509a7cbca53b6e22f3d67df
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: qedi: Initialize callback state before registration

This patch fixes a race condition where qedi_get_protocol_tlv_data() could
be called asynchronously by QED before the stats_lock and ll2_mtu were
initialized. It moves the callback registration to after these initializations
on the normal probe path.

> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 227ff7bd1bdc9..f394fcbb97e7e 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -2652,8 +2652,6 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  	snprintf(host_buf, sizeof(host_buf), "host_%d", qedi->shost->host_no);
>  	qedi_ops->common->set_name(qedi->cdev, host_buf);
>  
> -	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
> -
>  	memset(&params, 0, sizeof(params));
>  	params.mtu = DEF_PATH_MTU + IPV6_HDR_LEN + TCP_HDR_LEN;
>  	qedi->ll2_mtu = DEF_PATH_MTU;

[Severity: High]
This is a pre-existing issue, but are user-configured custom MTU settings
overwritten during hardware recovery here?

During a hardware recovery event, __qedi_probe() is invoked with
mode == QEDI_MODE_RECOVERY. This unconditional assignment to DEF_PATH_MTU
discards any custom MTU size (such as a 9000-byte jumbo MTU) that the user
may have configured.

> @@ -2670,6 +2668,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  		spin_lock_init(&qedi->task_idx_lock);
>  		mutex_init(&qedi->stats_lock);
>  	}
> +	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);

[Severity: High]
This isn't a bug introduced by this patch, but could registering the callbacks
here expose uninitialized work structures?

The qedi_cb_ops expose callback functions like
qedi_schedule_recovery_handler() to the QED core. This handler attempts to
schedule qedi->recovery_work, but INIT_DELAYED_WORK(&qedi->recovery_work, ...)
is not called until much later in the probe sequence.

If an asynchronous hardware event occurs in this window, it looks like
schedule_delayed_work() could be invoked on zero-filled memory.

>  	qedi_ops->ll2->register_cb_ops(qedi->cdev, &qedi_ll2_cb_ops, qedi);
>  	qedi_ops->ll2->start(qedi->cdev, &params);

[Severity: High]
This is a pre-existing issue, but does starting the hardware packet reception
here introduce a race condition?

If a packet arrives immediately, qedi_ll2_rx() calls
wake_up_process(qedi->ll2_recv_thread) while the thread pointer is still NULL.

Additionally, is it safe to ignore the return value of kthread_run() when it
creates qedi->ll2_recv_thread shortly after this?

If kthread_run() fails, it returns an ERR_PTR which is neither checked nor
handled. This could cause subsequent packets or kthread_stop() during teardown
to dereference the ERR_PTR.


[Severity: High]
This is a pre-existing issue, but does exposing the device to userspace before
crucial connection structures are allocated create a race condition?

In drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    if (iscsi_host_add(qedi->shost, &pdev->dev)) {
        ...
    }

    rc = qedi_alloc_uio_rings(qedi);
    ...
    rc = qedi_cm_alloc_mem(qedi);

iscsi_host_add() registers the host with sysfs, emitting a uevent. If
userspace responds by issuing a netlink command to create an endpoint,
qedi_ep_connect() assigns the endpoint to qedi->ep_tbl[iscsi_cid].

However, qedi_cm_alloc_mem() allocates qedi->ep_tbl after iscsi_host_add(),
so qedi->ep_tbl would be NULL during this race.


[Severity: High]
This is a pre-existing issue, but is it possible the error path in
__qedi_probe() fails to clean up the LL2 subsystem and kthread?

If an initialization step fails after the hardware packet reception and
receiver kthread are started:

drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    ...
stop_iscsi_func:
    qedi_ops->stop(qedi->cdev);
stop_slowpath:
    qedi_ops->common->slowpath_stop(qedi->cdev);
    ...

The error handler frees the qedi structure but seems to completely omit
stopping the hardware LL2 interface or killing the kthread, which could lead
to a use-after-free when they access the freed qedi structure.


[Severity: High]
This is a pre-existing issue, but would the error path for iscsi_host_add()
failure cause a crash?

drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    if (iscsi_host_add(qedi->shost, &pdev->dev)) {
        QEDI_ERR(&qedi->dbg_ctx,
                 "Could not add iscsi host\n");
        rc = -ENOMEM;
        goto remove_host;
    }

If iscsi_host_add() fails, the code jumps to remove_host and unconditionally
executes iscsi_host_remove(). Since the host was never successfully added to
the driver core, won't this attempt to unregister an uninitialized device?


[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check during
network packet reception in qedi_ll2_rx()?

drivers/scsi/qedi/qedi_main.c:qedi_ll2_rx():

    if (eh->h_proto == htons(ETH_P_8021Q)) {
        memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
        eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
        skb_reset_mac_header(skb);
    }

    /* Filter out non FIP/FCoE frames here to free them faster */
    if (eh->h_proto != htons(ETH_P_ARP) && ...

The code reads eh->h_proto without verifying skb->len >= ETH_HLEN. If the
protocol is 802.1Q (VLAN), it invokes skb_pull(skb, VLAN_HLEN).

If a malformed packet is smaller than VLAN_HLEN, skb_pull() returns NULL,
which is then unconditionally dereferenced in the subsequent check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910092405.1300129-1-runyu.xiao@seu.edu.cn?part=1