Re: [PATCH v4 5/6] nfc: s3fwrn5: support the S3NRN4V variant

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unprotected data race and state machine desynchronization due to ignored NCI command timeout.
--

--- Patch [5]: [PATCH v4 5/6] nfc: s3fwrn5: support the S3NRN4V variant ---
commit b57146ef7e5e76c557b5b6a3455b96f7592b3c94
Author: Jorijn van der Graaf <[email protected]>

nfc: s3fwrn5: support the S3NRN4V variant

This commit adds support for the S3NRN4V Samsung NFC controller variant.
It introduces a new initialization sequence and calibration data upload
path using the DUAL_OPTION proprietary command.

> diff --git a/drivers/nfc/s3fwrn5/nci.c b/drivers/nfc/s3fwrn5/nci.c
> index 5a9de11bbecea..f1877797d1365 100644
> --- a/drivers/nfc/s3fwrn5/nci.c
> +++ b/drivers/nfc/s3fwrn5/nci.c
[ ... ]
> @@ -117,3 +142,155 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
[ ... ]
> +int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
> +				  const char *hw_name, const char *sw_name)
> +{
[ ... ]
> +	/*
> +	 * Ask the chip for its current calibration versions and skip the
> +	 * upload when both already match the blobs; a mismatch or an
> +	 * unparseable answer means the upload proceeds. GET_VER answers with
> +	 * versions, not a status byte, so nci_prop_cmd()'s return carries no
> +	 * meaning here.
> +	 */
> +	sub_oid = NCI_PROP_DUAL_SUB_GET_VER;
> +	info->dual_rsp_len = 0;
> +	nci_prop_cmd(info->ndev, NCI_PROP_DUAL_OPTION, 1, &sub_oid);
> +	if (s3fwrn5_nci_dual_cal_current(info, hw_fw, sw_fw)) {

[Severity: High]
Could ignoring the return value of nci_prop_cmd() here cause a data race?

If the controller is slow or unresponsive, nci_prop_cmd() will return
-ETIMEDOUT. Because the error is ignored, the code immediately calls
s3fwrn5_nci_dual_cal_current() which reads info->dual_rsp_len and
info->dual_rsp.

Concurrently, if a late response arrives, the RX workqueue will execute
s3fwrn5_nci_dual_rsp():

s3fwrn5_nci_dual_rsp() {
	...
	info->dual_rsp_len = min_t(unsigned int, skb->len,
				   sizeof(info->dual_rsp));
	memcpy(info->dual_rsp, skb->data, info->dual_rsp_len);
	...
}

This locklessly overwrites the buffer while it is being read, potentially
leading to a torn read of the calibration version data.

Additionally, would a late response to this timed-out command erroneously
fulfill the subsequent START_UPDATE command, shifting the response stream by
one and breaking synchronization with the hardware?

> +		dev_dbg(dev, "calibration data already current\n");
> +		ret = 0;
> +		goto out;
> +	}
[ ... ]

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