Re: [PATCH net-next 1/6] eth: fbnic: move sensor read logic out of fbnic_mac

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

Pre-existing issues:
- [High] A positive firmware error code causes `fbnic_hwmon_sensor_read` to return a positive value to the hwmon core, which treats it as success and leaks an uninitialized stack variable.
- [High] Mailbox completions lack transaction IDs, allowing late firmware responses from timed-out requests to erroneously complete subsequent requests.
--

--- Patch [1]: [PATCH net-next 1/6] eth: fbnic: move sensor read logic out of fbnic_mac ---
commit 3d23a98846b8fe29f6da2d75356fdb3ad9559179
Author: Zinc Lim <[email protected]>

eth: fbnic: move sensor read logic out of fbnic_mac

The sensor read lived behind the fbnic_mac get_sensor op, but it is only
ever used by the hwmon subsystem. Move the read into fbnic_hwmon.c and
call it directly there, closer to where it is used, and drop the
now-unused get_sensor op from struct fbnic_mac. No functional change.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index def8598aceecf..6c8c66ab86c13 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -28,15 +28,67 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,

[ ... ]

> +	err = fbnic_fw_xmit_tsene_read_msg(fbd, fw_cmpl);
> +	if (err) {
> +		dev_err(fbd->dev,
> +			"Failed to transmit TSENE read msg, err %d\n",
> +			err);
> +		goto exit_free;
> +	}
> +
> +	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
> +		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");
> +		err = -ETIMEDOUT;
> +		goto exit_cleanup;
> +	}

[Severity: High]
This is a pre-existing issue, but does timing out and clearing the completion
slot expose the driver to late responses matching the next request?

If wait_for_completion_timeout() times out, the mailbox slot is cleared. Since
the matching logic in fbnic_fw_get_cmpl_by_type() uses only the message type:

	if (fbd->cmpl_data[i] &&
	    fbd->cmpl_data[i]->msg_type == msg_type) {

Will a delayed response to this request erroneously complete a subsequent
request of the same type?

> +
> +	/* Handle error returned by firmware */
> +	if (fw_cmpl->result) {
> +		err = fw_cmpl->result;
> +		dev_err(fbd->dev, "%s: Firmware returned error %d\n",
> +			__func__, err);
> +		goto exit_cleanup;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but what happens if the firmware
returns a positive error code in fw_cmpl->result?

Since this error code is propagated back to the hwmon core, and the hwmon
core only checks for negative error codes, will it treat a positive value
as success and leak an uninitialized stack variable (val) to userspace?

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