linux-kernel - Re: [PATCH] EISA: Increase length of device names

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: <202504070957.A97CCC289B@keescook>
Date: Mon, 7 Apr 2025 10:12:28 -0700
From: Kees Cook <kees@...nel.org>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Alejandro Colomar <alx@...nel.org>, Alex Elder <elder@...cstar.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Azeem Shaikh <azeemshaikh38@...il.com>,
	Alex Elder <elder@...nel.org>, Sumit Garg <sumit.garg@...nel.org>,
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] EISA: Increase length of device names

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