| Message ID | 20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] net/ncsi: fix buffer overflow in getting version id | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hello Jerry, Thank you for the patch. You should be able to follow progress on the Patchwork[0]. What upstream tree did you intend it for and why? It doesn't apply cleanly to net-next, that's for sure. More inline. On Thu, Feb 27, 2025 at 01:50:44PM +0800, Jerry C Chen wrote: > In NC-SI spec v1.2 section 8.4.44.2, the firmware name doesn't > need to be null terminated while its size occupies the full size > of the field. Right, the specification guarantees null-termination if there's enough space for it but also allows the firmware name to occupy all the 12 bytes and then it's not null-terminated. Have you seen such cards in the wild? It wouldn't harm mentioning specific examples in the commit message to probably help people searching for problems specific to them later. You can also consider adding Fixes: and Cc: stable tags if this bugfix solves a real issue and should be backported to stable kernels. > Fix the buffer overflow issue by adding one > additional byte for null terminator. This buffer is only written to by ncsi-rsp.c: memcpy(ncv->fw_name, rsp->fw_name, 12); hence there's no possibility of overflow. The real problem is the potential lack of the terminating NULL when it's later used by nla_put_string(skb, NCSI_CHANNEL_ATTR_VERSION_STR, nc->version.fw_name); which indeed expects a "NUL terminated string". But how exactly does your patch guarantee that the 13th byte of fw_name is going to be NUL is unclear. I suggest it's done explicitly in the code after memcpy. > WIWYNN PROPRIETARY > This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately. There should be nothing "proprietary or confidential" about your patches for upstream. It's not unlikely the maintainers will be ignoring patches from you containing this notice because they have no way to determine who is the intended recipient and what exactly is authorised. [0] https://patchwork.kernel.org/project/netdevbpf/patch/20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com/
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h index 4e0842df5234..fb918af74521 100644 --- a/net/ncsi/internal.h +++ b/net/ncsi/internal.h @@ -110,7 +110,7 @@ struct ncsi_channel_version { u8 update; /* NCSI version update */ char alpha1; /* NCSI version alpha1 */ char alpha2; /* NCSI version alpha2 */ - u8 fw_name[12]; /* Firmware name string */ + u8 fw_name[12+1]; /* Firmware name string */ u32 fw_version; /* Firmware version */ u16 pci_ids[4]; /* PCI identification */ u32 mf_id; /* Manufacture ID */
In NC-SI spec v1.2 section 8.4.44.2, the firmware name doesn't need to be null terminated while its size occupies the full size of the field. Fix the buffer overflow issue by adding one additional byte for null terminator. Signed-off-by: Jerry C Chen <Jerry_C_Chen@wiwynn.com> --- net/ncsi/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.25.1 WIWYNN PROPRIETARY This email (and any attachments) contains proprietary or confidential information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited. If you are not the intended recipient, please notify the sender and delete this email immediately.