[PATCH] EISA: Increase length of device names

[PATCH] EISA: Increase length of device names

[Kees Cook]

GCC 15's -Wunterminated-string-initialization warned about truncated
name strings. Instead of marking them with the "nonstring" attribute[1],
increase their length to correctly include enough space for the
terminating NUL character, as they are used with %s format specifiers.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Sumit Garg <sumit.garg@kernel.org>
Signed-off-by: Kees Cook <kees@kernel.org>
---
 drivers/eisa/eisa-bus.c | 2 +-
 include/linux/eisa.h    | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/eisa-bus.c b/drivers/eisa/eisa-bus.c
index cb586a362944..edceea083b98 100644
--- a/drivers/eisa/eisa-bus.c
+++ b/drivers/eisa/eisa-bus.c
@@ -21,7 +21,7 @@
 
 struct eisa_device_info {
 	struct eisa_device_id id;
-	char name[50];
+	char name[EISA_DEVICE_INFO_NAME_SIZE];
 };
 
 #ifdef CONFIG_EISA_NAMES
diff --git a/include/linux/eisa.h b/include/linux/eisa.h
index f98200cae637..c73a410bf88f 100644
--- a/include/linux/eisa.h
+++ b/include/linux/eisa.h
@@ -28,6 +28,8 @@
 #define EISA_CONFIG_ENABLED         1
 #define EISA_CONFIG_FORCED          2
 
+#define EISA_DEVICE_INFO_NAME_SIZE	51
+
 /* There is not much we can say about an EISA device, apart from
  * signature, slot number, and base address. dma_mask is set by
  * default to parent device mask..*/
@@ -41,7 +43,7 @@ struct eisa_device {
 	u64                   dma_mask;
 	struct device         dev; /* generic device */
 #ifdef CONFIG_EISA_NAMES
-	char		      pretty_name[50];
+	char		      pretty_name[EISA_DEVICE_INFO_NAME_SIZE];
 #endif
 };
 
-- 
2.34.1

Re: [PATCH] EISA: Increase length of device names

[Alex Elder]

On 3/10/25 5:24 PM, Kees Cook wrote:
> GCC 15's -Wunterminated-string-initialization warned about truncated
> name strings. Instead of marking them with the "nonstring" attribute[1],
> increase their length to correctly include enough space for the
> terminating NUL character, as they are used with %s format specifiers.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
> Cc: Alex Elder <elder@kernel.org>

This was interesting, but based on the bug text I suspect you
meant to address this to Alejandro Colomar, <alx@kernel.org>.

For what it's worth, it looks fine to me.

					-Alex

> Cc: Sumit Garg <sumit.garg@kernel.org>
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
>   drivers/eisa/eisa-bus.c | 2 +-
>   include/linux/eisa.h    | 4 +++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/eisa/eisa-bus.c b/drivers/eisa/eisa-bus.c
> index cb586a362944..edceea083b98 100644
> --- a/drivers/eisa/eisa-bus.c
> +++ b/drivers/eisa/eisa-bus.c
> @@ -21,7 +21,7 @@
>   
>   struct eisa_device_info {
>   	struct eisa_device_id id;
> -	char name[50];
> +	char name[EISA_DEVICE_INFO_NAME_SIZE];
>   };
>   
>   #ifdef CONFIG_EISA_NAMES
> diff --git a/include/linux/eisa.h b/include/linux/eisa.h
> index f98200cae637..c73a410bf88f 100644
> --- a/include/linux/eisa.h
> +++ b/include/linux/eisa.h
> @@ -28,6 +28,8 @@
>   #define EISA_CONFIG_ENABLED         1
>   #define EISA_CONFIG_FORCED          2
>   
> +#define EISA_DEVICE_INFO_NAME_SIZE	51
> +
>   /* There is not much we can say about an EISA device, apart from
>    * signature, slot number, and base address. dma_mask is set by
>    * default to parent device mask..*/
> @@ -41,7 +43,7 @@ struct eisa_device {
>   	u64                   dma_mask;
>   	struct device         dev; /* generic device */
>   #ifdef CONFIG_EISA_NAMES
> -	char		      pretty_name[50];
> +	char		      pretty_name[EISA_DEVICE_INFO_NAME_SIZE];
>   #endif
>   };
>   

Re: [PATCH] EISA: Increase length of device names

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

Hi Kees, Alex,

On Sat, Mar 15, 2025 at 09:27:36AM -0500, Alex Elder wrote:
> On 3/10/25 5:24 PM, Kees Cook wrote:
> > GCC 15's -Wunterminated-string-initialization warned about truncated
> > name strings. Instead of marking them with the "nonstring" attribute[1],
> > increase their length to correctly include enough space for the
> > terminating NUL character, as they are used with %s format specifiers.

It might be interesting to mention where they are used with %s.

> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
> > Cc: Alex Elder <elder@kernel.org>
> 
> This was interesting, but based on the bug text I suspect you
> meant to address this to Alejandro Colomar, <alx@kernel.org>.

Thanks!

> For what it's worth, it looks fine to me.

LGTM too.  Assuming that changing the size of the arrays doesn't break
something else, it looks good.


Have a lovely day!
Alex

> 					-Alex
> 
> > Cc: Sumit Garg <sumit.garg@kernel.org>
> > Signed-off-by: Kees Cook <kees@kernel.org>
> > ---
> >   drivers/eisa/eisa-bus.c | 2 +-
> >   include/linux/eisa.h    | 4 +++-
> >   2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/eisa/eisa-bus.c b/drivers/eisa/eisa-bus.c
> > index cb586a362944..edceea083b98 100644
> > --- a/drivers/eisa/eisa-bus.c
> > +++ b/drivers/eisa/eisa-bus.c
> > @@ -21,7 +21,7 @@
> >   struct eisa_device_info {
> >   	struct eisa_device_id id;
> > -	char name[50];
> > +	char name[EISA_DEVICE_INFO_NAME_SIZE];
> >   };
> >   #ifdef CONFIG_EISA_NAMES
> > diff --git a/include/linux/eisa.h b/include/linux/eisa.h
> > index f98200cae637..c73a410bf88f 100644
> > --- a/include/linux/eisa.h
> > +++ b/include/linux/eisa.h
> > @@ -28,6 +28,8 @@
> >   #define EISA_CONFIG_ENABLED         1
> >   #define EISA_CONFIG_FORCED          2
> > +#define EISA_DEVICE_INFO_NAME_SIZE	51
> > +
> >   /* There is not much we can say about an EISA device, apart from
> >    * signature, slot number, and base address. dma_mask is set by
> >    * default to parent device mask..*/
> > @@ -41,7 +43,7 @@ struct eisa_device {
> >   	u64                   dma_mask;
> >   	struct device         dev; /* generic device */
> >   #ifdef CONFIG_EISA_NAMES
> > -	char		      pretty_name[50];
> > +	char		      pretty_name[EISA_DEVICE_INFO_NAME_SIZE];
> >   #endif
> >   };
> 

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] EISA: Increase length of device names

[Maciej W. Rozycki]

On Sat, 15 Mar 2025, Alejandro Colomar wrote:

> > > GCC 15's -Wunterminated-string-initialization warned about truncated
> > > name strings. Instead of marking them with the "nonstring" attribute[1],
> > > increase their length to correctly include enough space for the
> > > terminating NUL character, as they are used with %s format specifiers.
> 
> It might be interesting to mention where they are used with %s.

 Indeed.  I seem to be missing something here as I can't see an issue in 
reality:

# cat /proc/ioports | sed -n '/EISA/,$p'
0c80-0c83 : 486EI EISA System Board
5000-50ff : DEC FDDIcontroller/EISA Adapter
  5000-503f : defxx
  5040-5043 : defxx
5400-54ff : DEC FDDIcontroller/EISA Adapter
5800-58ff : DEC FDDIcontroller/EISA Adapter
5c00-5cff : DEC FDDIcontroller/EISA Adapter
  5c80-5cbf : defxx
6000-60ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
6400-64ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
6800-68ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
6c00-6cff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
8000-80ff : 3Com 3C509-Combo Network Adapter
  8000-800f : 3c579-eisa
8400-84ff : 3Com 3C509-Combo Network Adapter
8800-88ff : 3Com 3C509-Combo Network Adapter
8c00-8cff : 3Com 3C509-Combo Network Adapter
# 

nor why incrementing the length specifically to 51 (where eisa.ids names 
are up to 73 characters; one of the longer entries can be seen truncated 
above) is going to change anything here.  Overall since the string length 
is fixed I'd expect just using `%.50s' instead.

> > For what it's worth, it looks fine to me.
> 
> LGTM too.  Assuming that changing the size of the arrays doesn't break
> something else, it looks good.

 ISTM increasing to 74 instead might make more sense (I don't know what 
the actual maximum size was according to the ECU standard, but it might 
not be that we'll ever add any new entries to our list), once the origin 
of the problem is known, though I think we need to evaluate what effect 
such a change will have on the size of the compiled kernel.

  Maciej

Re: [PATCH] EISA: Increase length of device names

[Kees Cook]

On Sat, Mar 15, 2025 at 09:27:36AM -0500, Alex Elder wrote:
> On 3/10/25 5:24 PM, Kees Cook wrote:
> > GCC 15's -Wunterminated-string-initialization warned about truncated
> > name strings. Instead of marking them with the "nonstring" attribute[1],
> > increase their length to correctly include enough space for the
> > terminating NUL character, as they are used with %s format specifiers.
> > 
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
> > Cc: Alex Elder <elder@kernel.org>
> 
> This was interesting, but based on the bug text I suspect you
> meant to address this to Alejandro Colomar, <alx@kernel.org>.

I think you got tagged because of the Reviewed-by on commit d69d80484598
("driver core: have match() callback in struct bus_type take a const *")
which touched include/linux/eisa.h.

> For what it's worth, it looks fine to me.

Thanks!

-- 
Kees Cook

Re: [PATCH] EISA: Increase length of device names

[Kees Cook]

On Thu, Apr 03, 2025 at 12:23:31AM +0100, Maciej W. Rozycki wrote:
> On Sat, 15 Mar 2025, Alejandro Colomar wrote:
> 
> > > > GCC 15's -Wunterminated-string-initialization warned about truncated
> > > > name strings. Instead of marking them with the "nonstring" attribute[1],
> > > > increase their length to correctly include enough space for the
> > > > terminating NUL character, as they are used with %s format specifiers.
> > 
> > It might be interesting to mention where they are used with %s.
> 
>  Indeed.  I seem to be missing something here as I can't see an issue in 
> reality:
> 
> # cat /proc/ioports | sed -n '/EISA/,$p'
> 0c80-0c83 : 486EI EISA System Board
> 5000-50ff : DEC FDDIcontroller/EISA Adapter
>   5000-503f : defxx
>   5040-5043 : defxx
> 5400-54ff : DEC FDDIcontroller/EISA Adapter
> 5800-58ff : DEC FDDIcontroller/EISA Adapter
> 5c00-5cff : DEC FDDIcontroller/EISA Adapter
>   5c80-5cbf : defxx
> 6000-60ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
> 6400-64ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
> 6800-68ff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
> 6c00-6cff : Network Peripherals NP-EISA-3E Enhanced FDDI Inte
> 8000-80ff : 3Com 3C509-Combo Network Adapter
>   8000-800f : 3c579-eisa
> 8400-84ff : 3Com 3C509-Combo Network Adapter
> 8800-88ff : 3Com 3C509-Combo Network Adapter
> 8c00-8cff : 3Com 3C509-Combo Network Adapter
> # 
> 
> nor why incrementing the length specifically to 51 (where eisa.ids names 
> are up to 73 characters; one of the longer entries can be seen truncated 
> above) is going to change anything here.  Overall since the string length 
> is fixed I'd expect just using `%.50s' instead.

These are produced from:

        seq_printf(m, "%*s%0*llx-%0*llx : %s\n", ..., r->name);

which is struct resource::name, which is unbounded.

> 
> > > For what it's worth, it looks fine to me.
> > 
> > LGTM too.  Assuming that changing the size of the arrays doesn't break
> > something else, it looks good.
> 
>  ISTM increasing to 74 instead might make more sense (I don't know what 
> the actual maximum size was according to the ECU standard, but it might 

The only mention of names I could find in the EISA spec just says,
"The NAME field contains text that describes the board. The MFR field
contains the full name of the board manufacturer." This is in the
configuration files, though, so it seems unbounded.

I suspect the right choice is to use eisa.ids as the length guide, as
you suggest.

> not be that we'll ever add any new entries to our list), once the origin 
> of the problem is known, though I think we need to evaluate what effect 
> such a change will have on the size of the compiled kernel.

I also see this hard limit of 50 in the Makefile:

# Ugly hack to get DEVICE_NAME_SIZE value...
DEVICE_NAME_SIZE = 50

$(obj)/eisa-bus.o: $(obj)/devlist.h

quiet_cmd_eisaid = GEN     $@
      cmd_eisaid = sed -e '/^\#/D' -e 's/^\([[:alnum:]]\{7\}\) \+"\([^"]\{1,$(DEVICE_NAME_SIZE)\}\).*"/EISA_DEVINFO ("\1", "\2"),/' $< > $@

The 50 was chosen seemingly at random in commit ca52a49846f1 ("driver
core: remove DEVICE_NAME_SIZE define").

I think that limit can be removed entirely.

-- 
Kees Cook