linux-kernel - Re: [PATCH v1] net/ncsi: fix buffer overflow in getting version id

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z8AokYA+ZNsxnHaG@home.paul.comp>
Date: Thu, 27 Feb 2025 11:55:45 +0300
From: Paul Fertser <fercerpav@...il.com>
To: Jerry C Chen <Jerry_C_Chen@...ynn.com>
Cc: patrick@...cx.xyz, Samuel Mendoza-Jonas <sam@...dozajonas.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] net/ncsi: fix buffer overflow in getting version id

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/