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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -117,3 +142,155 @@ int s3fwrn5_nci_rf_configure(struct s3fwrn5_info *info, const char *fw_name)
[ ... ]
quoted hunk ↗ jump to hunk
+int s3fwrn5_nci_rf_configure_dual(struct s3fwrn5_info *info,
+ const char *hw_name, const char *sw_name)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ /*
+ * 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?
quoted hunk ↗ jump to hunk
+ dev_dbg(dev, "calibration data already current\n");
+ ret = 0;
+ goto out;
+ }
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260807021001.239389-1-jorijnvdgraaf@catcrafts.net?part=5