[PATCH v1] net/ncsi: fix buffer overflow in getting version id

[PATCH v1] net/ncsi: fix buffer overflow in getting version id

[Jerry C Chen]

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(-)

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                     */
--
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.

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

[Paul Fertser]

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/

[PATCH v1] net/ncsi: fix buffer overflow in getting version id

[Jerry C Chen]

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 +-
 net/ncsi/ncsi-rsp.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 2c260f33b55c..ad1f671ffc37 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                     */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 8668888c5a2f..d5ed80731e89 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -775,6 +775,7 @@ static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
        ncv->alpha1 = rsp->alpha1;
        ncv->alpha2 = rsp->alpha2;
        memcpy(ncv->fw_name, rsp->fw_name, 12);
+       ncv->fw_name[12] = '\0';
        ncv->fw_version = ntohl(rsp->fw_version);
        for (i = 0; i < ARRAY_SIZE(ncv->pci_ids); i++)
                ncv->pci_ids[i] = ntohs(rsp->pci_ids[i]);
--
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.

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

[Paul Fertser]

Hello Jerry,

This looks like an updated version of your previous patch[0] but you
have forgotten to increase the number in the Subject. You have also
forgotten to reply and take into account /some/ of the points I raised
in the review.

On Thu, May 15, 2025 at 04:34:47PM +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. Fix the buffer overflow issue by adding one
> additional byte for null terminator.
...

Please give an answer to every comment I made for your previous patch
version and either make a corresponding change or explain why exactly
you disagree.

Also please stop sending any and all "proprietary or confidential
information".

[0] https://patchwork.kernel.org/project/netdevbpf/patch/20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com/

RE: [PATCH v1] net/ncsi: fix buffer overflow in getting version id

[Jerry C Chen/WYHQ/Wiwynn]

Hi Paul,

Sorry for late replay, it takes some effort to change company policy of the proprietary.
For the questions:
1. What upstream tree did you intend it for and why?
 - Linux mainline
  We are developing openBMC with kernel-6.6.
  For submitting patch to kernel-6.6 stable tree, it should exist in mainline first.
  Reference: https://github.com/openbmc/linux/commits/dev-6.6/

2. 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.
 - This NIC is developed by META terminus team and the problematic string is:
 The channel Version Str : 24.12.08-000
 I will update it to commit message later.

> -----Original Message-----
> From: Paul Fertser <fercerpav@gmail.com>
> Sent: Thursday, May 15, 2025 5:04 PM
> To: Jerry C Chen/WYHQ/Wiwynn <Jerry_C_Chen@wiwynn.com>
> Cc: patrick@stwcx.xyz; Samuel Mendoza-Jonas <sam@mendozajonas.com>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1] net/ncsi: fix buffer overflow in getting version id
> 
>  [External Sender]
> 
> Hello Jerry,
> 
> This looks like an updated version of your previous patch[0] but you have
> forgotten to increase the number in the Subject. You have also forgotten to
> reply and take into account /some/ of the points I raised in the review.
> 
> On Thu, May 15, 2025 at 04:34:47PM +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.
> > Fix the buffer overflow issue by adding one additional byte for null
> > terminator.
> ...
> 
> Please give an answer to every comment I made for your previous patch
> version and either make a corresponding change or explain why exactly you
> disagree.
> 
> Also please stop sending any and all "proprietary or confidential information".
> 
> [0]
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/netdevbpf/p
> atch/20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com/__;!!ObgLwW8
> oGsQ!nQ0Zkq6AxOKAJHbUUrTRnNI8fJNt7itufBwUXkkZU1-yfFo3h6Vm55K_mqr
> 5Ur5kw9wE9cMVgIdoGCL3u2DhhqA$

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

[Paul Fertser]

Hi Jerry,

On Fri, May 23, 2025 at 07:32:26AM +0000, Jerry C Chen/WYHQ/Wiwynn wrote:
> Sorry for late replay, it takes some effort to change company policy of the proprietary.

I can imagine! However it's not necessary to send patches from
corporate e-mail address via the corporate mail server, you can just
send from your own personal account with the appropriate From:
specification to attribute it to your corporate address[0].

> For the questions:

Please consider just using standard inline method of replying in the
future, letting your MUA quote the original message for context
properly.

> 1. What upstream tree did you intend it for and why?
>  - Linux mainline
>   We are developing openBMC with kernel-6.6.
>   For submitting patch to kernel-6.6 stable tree, it should exist in mainline first.
>   Reference: https://github.com/openbmc/linux/commits/dev-6.6/

Indeed, and the process of submitting to mainline implies that for
each subsystem there's a tree which subsystem maintainer(s) use for
the integration and which is later offered as a the pull request for
the upcoming version, usually it's called {subsystem}-next (also such
trees get tested together being merged into linux-next regularly). I
guess in this case you should make sure your patch applies to net-next
(and makes sense there). Neither the current submission[1] nor the
previous one[2] were applicable (see "netdev/tree_selection success
Guessing tree name failed - patch did not apply" and indeed I tried to
"git am" it manually to what was "net-next" back then and it failed.

> 2. 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.
>  - This NIC is developed by META terminus team and the problematic string is:
>  The channel Version Str : 24.12.08-000
>  I will update it to commit message later.

I see, thank you. Sigh, this 12 characters limit doesn't seem to make
much sense, too restrictive to fit a useful part of "git describe
--tags" even, but it is what it is...

[0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#from-line
[1] https://patchwork.kernel.org/project/netdevbpf/patch/20250515083448.3511588-1-Jerry_C_Chen@wiwynn.com/
[2] https://patchwork.kernel.org/project/netdevbpf/patch/20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com/

RE: [PATCH v1] net/ncsi: fix buffer overflow in getting version id

[Jerry C Chen/WYHQ/Wiwynn]

Hi Paul,

Thank for your reply and suggestions.
Since the net-next is closed now: https://netdev.bots.linux.dev/net-next.html
I will submit new patch(new mail thread) while it re-open.
BTW, seems the re-open day is wrong: "Closed until April 7th", may I know which day will it open? Thanks.

> -----Original Message-----
> From: Paul Fertser <fercerpav@gmail.com>
> Sent: Tuesday, May 27, 2025 4:15 AM
> To: Jerry C Chen/WYHQ/Wiwynn <Jerry_C_Chen@wiwynn.com>
> Cc: patrick@stwcx.xyz; Samuel Mendoza-Jonas <sam@mendozajonas.com>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Simon Horman <horms@kernel.org>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: Re: [PATCH v1] net/ncsi: fix buffer overflow in getting version id
> 
>  [External Sender]
> 
> Hi Jerry,
> 
> On Fri, May 23, 2025 at 07:32:26AM +0000, Jerry C Chen/WYHQ/Wiwynn
> wrote:
> > Sorry for late replay, it takes some effort to change company policy of the
> proprietary.
> 
> I can imagine! However it's not necessary to send patches from corporate
> e-mail address via the corporate mail server, you can just send from your own
> personal account with the appropriate From:
> specification to attribute it to your corporate address[0].
> 
> > For the questions:
> 
> Please consider just using standard inline method of replying in the future,
> letting your MUA quote the original message for context properly.
> 
> > 1. What upstream tree did you intend it for and why?
> >  - Linux mainline
> >   We are developing openBMC with kernel-6.6.
> >   For submitting patch to kernel-6.6 stable tree, it should exist in mainline
> first.
> >   Reference:
> > https://urldefense.com/v3/__https://github.com/openbmc/linux/commits/d
> >
> ev-6.6/__;!!ObgLwW8oGsQ!lfBTzKPEhZloN2Uwc85U1tsMWw41Yq8dTkG9oxjckT
> okV1
> > 1cR1AQaKFRrIwg9G02e1CpUC_SUX9aFCoEREqXU-Q$
> 
> Indeed, and the process of submitting to mainline implies that for each
> subsystem there's a tree which subsystem maintainer(s) use for the integration
> and which is later offered as a the pull request for the upcoming version,
> usually it's called {subsystem}-next (also such trees get tested together being
> merged into linux-next regularly). I guess in this case you should make sure
> your patch applies to net-next (and makes sense there). Neither the current
> submission[1] nor the previous one[2] were applicable (see
> "netdev/tree_selection success Guessing tree name failed - patch did not
> apply" and indeed I tried to "git am" it manually to what was "net-next" back
> then and it failed.
> 
> > 2. 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.
> >  - This NIC is developed by META terminus team and the problematic string
> is:
> >  The channel Version Str : 24.12.08-000  I will update it to commit
> > message later.
> 
> I see, thank you. Sigh, this 12 characters limit doesn't seem to make much
> sense, too restrictive to fit a useful part of "git describe --tags" even, but it is
> what it is...
> 
> [0]
> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/
> submitting-patches.html*from-line__;Iw!!ObgLwW8oGsQ!lfBTzKPEhZloN2Uwc8
> 5U1tsMWw41Yq8dTkG9oxjckTokV11cR1AQaKFRrIwg9G02e1CpUC_SUX9aFCoE
> FjZPgHI$
> [1]
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/netdevbpf/p
> atch/20250515083448.3511588-1-Jerry_C_Chen@wiwynn.com/__;!!ObgLwW8
> oGsQ!lfBTzKPEhZloN2Uwc85U1tsMWw41Yq8dTkG9oxjckTokV11cR1AQaKFRrIw
> g9G02e1CpUC_SUX9aFCoEd6rkbGA$
> [2]
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/netdevbpf/p
> atch/20250227055044.3878374-1-Jerry_C_Chen@wiwynn.com/__;!!ObgLwW8
> oGsQ!lfBTzKPEhZloN2Uwc85U1tsMWw41Yq8dTkG9oxjckTokV11cR1AQaKFRrIw
> g9G02e1CpUC_SUX9aFCoEVccOzuI$