ethtool generate a buffer overflow in strlen

ethtool generate a buffer overflow in strlen

[Jules Maselbas]

Hi,

I've come across this following image of a kernel BUG stack strace:
https://twitter.com/minut_e/status/1550139692615667715/photo/1

here is an ocr of the image above:
root@reform:~# ethtool -S enp0s0f0
[473.215343] detected buffer overflow in strlen
[473.219873] ------------[ cut here ]------------
[473.224502] kernel BUG at lib/string.c:1149!
[473.228785] Internal error: Oops - BUG: @ [#1] PREEMPT SMP
[473.234288] Modules linked in:
[473.237350] CPU: 1 PID: 1348 Comm: ethtool Not tainted 5.13.0-rc1+ #37
[473.243900] Hardware name: MNT Reform 2 with LS1028A (DT)
[473.249313] pstate: 6000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[473.255339] pc : fortify_panic+0x20/0x24
[473.259281] Ir : fortify_panic+0x20/0x24
[473.263214] sp : ffffffc01093bb20
[473.266534] x29: ffffffc01093bb20 x28: ffffffa001591f00 x27: 0000000000000000
[473.273699] x26: 0000000000000000 x25: ffffffa0020fe840 x24: 0000000000000002
[473.280863] x23: ffffffe9f5b66408 x22: ffffffe9f5b671e4 x21: ffffffe9f5b66bd8
[473.288027] x20: 0000000000000020 x19: ffffffc0100b0a60 x18: 0000000000000000
[473.295189] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000030
[473.302353] x14: ffffffffffffffff x13: ffffffc09093b817 x12: ffffffc01093b81f
[473.309515] x11: ffffffe9f6815850 x10: eeaeaeaeFFTFTeG x9 : ffffffe9f4920c50
[473.316679] x8 : ffffffe9f67bd850 x7 : ffffffe9f6815850 x6 : 0000000000000000
[473.323841] x5 : 0000000000000000 x4 : ffffffal7efb79a0 x3 : 0000000000000000
[473.331003] x2 : 0000000000000000 x1 : ffffffade1591fe0 xe : 0000000000000022
[473.338166] Call trace:
[473.340614]  fortify_panic+0x20/0x24
[473.344198]  enetc_get_ethtool_stats+8x0/0x21c
[473.348656]  ethtool_get_strings+0x360/0x394
[473.352939]  dev_ethtool+0x1194/0x212c
[473.356696]  dev_ioctl+0x4f4/8x5f0
[473.360107]  sock_do_ioctl+8x104/0x280
[473.363868]  sock_ioctl+0x294/0x484
[473.367364]  __arm64_sys_ioctl+0xb4/0x100
[473.371386]  invoke_syscall+0x50/0x120
[473.375146]  e10_svc_common.constprop.0+0x4c/0xd4
[473.379865]  do_e10_svc+0x30/0x9c
[473.383188]  e10_svc+0x2c/0x54
[473.386248]  e10_sync_handler+0x1a4/0x1bd
[473.390266]  e10_sync+0x198/0x1c8
[473.393590] Code: aa0003e1 912b4040 910003fd 97fff04b (d4218000)
[473.399702] ---[ end trace e4d82f308db974e2 ]---
[473.404331] note: ethtool[1340] exited with preempt_count 1
Segmentation faul [473.411589] ---- ----[ cut here ]------------
lt
---

There is suspicious lines in the file drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
   { ENETC_PM0_R1523X, "MAC rx 1523 to max-octet packets" },
and:
   { ENETC_PM0_T1523X, "MAC tx 1523 to max-octet packets" },

Where the string length is actually greater than 32 bytes which is more
than the reserved space for the name. This structure is defined as
follow:
    static const struct {
        int reg;
        char name[ETH_GSTRING_LEN];
    } enetc_port_counters[] = { ...

In the function enetc_get_strings(), there is a strlcpy call on the
counters names which in turns calls strlen on the src string, causing
an out-of-bound read, at least out-of the string.

I am not sure that's what caused the BUG, as I don't really know how
fortify works but I thinks this might only be visible when fortify is
enabled.

I am not sure on how to fix this issue, maybe use `char *` instead of
an byte array.

Best,
Jules

Re: ethtool generate a buffer overflow in strlen

[Jakub Kicinski]

On Fri, 22 Jul 2022 19:37:46 +0200 Jules Maselbas wrote:
> There is suspicious lines in the file drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
>    { ENETC_PM0_R1523X, "MAC rx 1523 to max-octet packets" },
> and:
>    { ENETC_PM0_T1523X, "MAC tx 1523 to max-octet packets" },
> 
> Where the string length is actually greater than 32 bytes which is more
> than the reserved space for the name. This structure is defined as
> follow:
>     static const struct {
>         int reg;
>         char name[ETH_GSTRING_LEN];
>     } enetc_port_counters[] = { ...
> 
> In the function enetc_get_strings(), there is a strlcpy call on the
> counters names which in turns calls strlen on the src string, causing
> an out-of-bound read, at least out-of the string.
> 
> I am not sure that's what caused the BUG, as I don't really know how
> fortify works but I thinks this might only be visible when fortify is
> enabled.
> 
> I am not sure on how to fix this issue, maybe use `char *` instead of
> an byte array.

Thanks for the report!

I'd suggest to just delete the RMON stats in the unstructured API
in this driver and report them via
 
	ethtool -S eth0 --groups rmon

No point trying to figure out a way to make the old API more
resilient IMO when we have an alternative.

Re: ethtool generate a buffer overflow in strlen

[Jules Maselbas]

On Fri, Jul 22, 2022 at 02:29:42PM -0700, Jakub Kicinski wrote:
> On Fri, 22 Jul 2022 19:37:46 +0200 Jules Maselbas wrote:
> > There is suspicious lines in the file drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
> >    { ENETC_PM0_R1523X, "MAC rx 1523 to max-octet packets" },
> > and:
> >    { ENETC_PM0_T1523X, "MAC tx 1523 to max-octet packets" },
> > 
> > Where the string length is actually greater than 32 bytes which is more
> > than the reserved space for the name. This structure is defined as
> > follow:
> >     static const struct {
> >         int reg;
> >         char name[ETH_GSTRING_LEN];
> >     } enetc_port_counters[] = { ...
> > 
> > In the function enetc_get_strings(), there is a strlcpy call on the
> > counters names which in turns calls strlen on the src string, causing
> > an out-of-bound read, at least out-of the string.
> > 
> > I am not sure that's what caused the BUG, as I don't really know how
> > fortify works but I thinks this might only be visible when fortify is
> > enabled.
> > 
> > I am not sure on how to fix this issue, maybe use `char *` instead of
> > an byte array.
> 
> Thanks for the report!
Thanks for the replie :)

> I'd suggest to just delete the RMON stats in the unstructured API
> in this driver and report them via
>  
> 	ethtool -S eth0 --groups rmon
I am not familiar with ethtool: I don't understand what you're
suggesting. Would you mind giving some hints/links to what RMON stats
are?


> No point trying to figure out a way to make the old API more
> resilient IMO when we have an alternative.
I was not thinking of changing the API but simply the structure to use
a string pointer instead of an array, this will make strings in the
enetc_port_counters properly nul terminated.
However the name will still be truncated when copied by the _get_strings
function... but it will not BUG on strlen.

Best,
Jules

RE: ethtool generate a buffer overflow in strlen

[Claudiu Manoil]

> -----Original Message-----
> From: Jules Maselbas <jmaselbas@kalray.eu>
> Sent: Monday, July 25, 2022 3:20 PM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; David S. Miller
> <davem@davemloft.net>; netdev@vger.kernel.org
> Subject: Re: ethtool generate a buffer overflow in strlen
> 
> On Fri, Jul 22, 2022 at 02:29:42PM -0700, Jakub Kicinski wrote:
> > On Fri, 22 Jul 2022 19:37:46 +0200 Jules Maselbas wrote:
> > > There is suspicious lines in the file
> drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
> > >    { ENETC_PM0_R1523X, "MAC rx 1523 to max-octet packets" },
> > > and:
> > >    { ENETC_PM0_T1523X, "MAC tx 1523 to max-octet packets" },
> > >
> > > Where the string length is actually greater than 32 bytes which is more
> > > than the reserved space for the name. This structure is defined as
> > > follow:
> > >     static const struct {
> > >         int reg;
> > >         char name[ETH_GSTRING_LEN];
> > >     } enetc_port_counters[] = { ...
> > >
> > > In the function enetc_get_strings(), there is a strlcpy call on the
> > > counters names which in turns calls strlen on the src string, causing
> > > an out-of-bound read, at least out-of the string.
> > >
> > > I am not sure that's what caused the BUG, as I don't really know how
> > > fortify works but I thinks this might only be visible when fortify is
> > > enabled.
> > >
> > > I am not sure on how to fix this issue, maybe use `char *` instead of
> > > an byte array.
> >
> > Thanks for the report!
> Thanks for the replie :)
> 
> > I'd suggest to just delete the RMON stats in the unstructured API
> > in this driver and report them via
> >
> > 	ethtool -S eth0 --groups rmon
> I am not familiar with ethtool: I don't understand what you're
> suggesting. Would you mind giving some hints/links to what RMON stats
> are?
> 

I can do it if you're patient.

Re: ethtool generate a buffer overflow in strlen

[Jules Maselbas]

On Mon, Jul 25, 2022 at 12:26:40PM +0000, Claudiu Manoil wrote:
> 
> 
> > -----Original Message-----
> > From: Jules Maselbas <jmaselbas@kalray.eu>
> > Sent: Monday, July 25, 2022 3:20 PM
> > To: Jakub Kicinski <kuba@kernel.org>
> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; David S. Miller
> > <davem@davemloft.net>; netdev@vger.kernel.org
> > Subject: Re: ethtool generate a buffer overflow in strlen
> > 
> > On Fri, Jul 22, 2022 at 02:29:42PM -0700, Jakub Kicinski wrote:
> > > On Fri, 22 Jul 2022 19:37:46 +0200 Jules Maselbas wrote:
> > > > There is suspicious lines in the file
> > drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:
> > > >    { ENETC_PM0_R1523X, "MAC rx 1523 to max-octet packets" },
> > > > and:
> > > >    { ENETC_PM0_T1523X, "MAC tx 1523 to max-octet packets" },
> > > >
> > > > Where the string length is actually greater than 32 bytes which is more
> > > > than the reserved space for the name. This structure is defined as
> > > > follow:
> > > >     static const struct {
> > > >         int reg;
> > > >         char name[ETH_GSTRING_LEN];
> > > >     } enetc_port_counters[] = { ...
> > > >
> > > > In the function enetc_get_strings(), there is a strlcpy call on the
> > > > counters names which in turns calls strlen on the src string, causing
> > > > an out-of-bound read, at least out-of the string.
> > > >
> > > > I am not sure that's what caused the BUG, as I don't really know how
> > > > fortify works but I thinks this might only be visible when fortify is
> > > > enabled.
> > > >
> > > > I am not sure on how to fix this issue, maybe use `char *` instead of
> > > > an byte array.
> > >
> > > Thanks for the report!
> > Thanks for the replie :)
> > 
> > > I'd suggest to just delete the RMON stats in the unstructured API
> > > in this driver and report them via
> > >
> > > 	ethtool -S eth0 --groups rmon
> > I am not familiar with ethtool: I don't understand what you're
> > suggesting. Would you mind giving some hints/links to what RMON stats
> > are?
> > 
> 
> I can do it if you're patient.
I am patient :]