On Thu, Sep 10, 2026 at 11:43:32AM +0200, Michal Simek wrote:
>
>
> On 9/10/26 11:30, Maarten Brock wrote:
> > Hello Michal,
> >
> > > -----Original Message-----
> > > From: Michal Simek <[email protected]>
> > > Sent: Thursday 10 September 2026 9:59
> > >
> > > On 9/10/26 09:56, Maarten Brock wrote:
> > > > Hello Sriram and Drew,
> > > >
> > > > It seems to me that the limit of 50 for MAX_NAME_LENGTH is pretty
> > > > arbitrary.
> > > > If you expect it to easily overflow, why not make that larger as well?
> > >
> > > do you have a case where your board, carrier cards will require more then
> > > 50
> > > chars? This has been designed for Kria SOM where you have name in board
> > > eeprom
> > > and carrier card eeprom and it is never over 50 chars.
> >
> > No, I do not. I was just following the logic in the patch comments.
> >
> > > -----Original Message-----
> > > From: Michal Simek <[email protected]>
> > > Sent: Thursday 10 September 2026 10:04
> > >
> > > On 9/9/26 21:20, Sriram Sriram wrote:
> > > > From: Drew Kluemke <[email protected]>
> > > >
> > > > board_name_decode() assembles the board name into a calloc'd buffer of
> > > > MAX_NAME_LENGTH (50) bytes using strcat(). It appends CONFIG_SYS_BOARD
> > > > and then, for every detected EEPROM descriptor, "-", desc->name (up to
> > > > 16 characters), "-rev" and desc->revision (up to 8 characters).
> > > >
> > > > The length check runs only after the loop, so it cannot prevent the
> > > > overflow it documents. Each descriptor contributes up to 29 characters,
> > > > so a base board plus a carrier card already exceeds the buffer, and the
> > > > data being concatenated comes from the EEPROM.
> >
> > To me this indicates it is very easy to overflow the buffer.
> >
> > I have not followed where the result of board_name_decode() goes
> > and whether 50 is a logical bound there. But since it is locally defined
> > without any comments it seems an arbitrary choice.
> >
> > And if both these assumptions are true (expect overflow and arbitrary bound)
> > then it is probably a good idea to use a larger bound.
>
> It is used for DTB reselection to described current board setup.
> If you look at arch/arm/dts/zynqmp-binman-*
> the maximum amount of chars used is 37. There is still 13 chars left.
>
> Code is prepared for covering board, carrier cards but also extension cards
> like FMCs by simply pointing via nvmem alias to whatever EEPROM in the
> system. I am not saying we would need more space but as of today I am not
> aware about any usecase which requires more. In any case if something hit
> that limit panic is thrown and we can extend it but extending it just in
> case make little sense to me.
>
> Thanks,
> Michal
>
>
> I actually got a path (c&p below) for the same issue some days ago
> internally and also reviewed it already. But unfortunately it wasn't
> sent out yet. The difference is that it is checking len at the end of
> every loop to get out of it earlier.
Please drop my 1/2 and take Pranav's instead. It is the same fix with
better failure behaviour -- detecting truncation inside the loop and
panicking there is more useful than my version, which truncated
silently and only caught the boundary case afterwards -- and it carries
the Fixes: tag mine was missing.
I did check it against U-Boot's strlcat(), which returns
min(strlen(dest), size) + strlen(src), and the per-iteration check
holds up: once the buffer is full, every later strlcat() in the same
iteration returns 49 + strlen(src) >= 50, and every append site has a
non-empty src (the "-" is unconditional), so the truncation indication
cannot be lost before the check is reached. A name of exactly 49
characters still passes, as it does today. Happy to give it a
Reviewed-by once it hits the list.
On the MAX_NAME_LENGTH question:
> It seems to me that the limit of 50 for MAX_NAME_LENGTH is pretty
> arbitrary. If you expect it to easily overflow, why not make that
> larger as well?
That is my commit message overselling it, sorry. "Each descriptor
contributes up to 29 characters" is the theoretical maximum of the
EEPROM fields (name[16] + revision[8] + "-" + "-rev"), not what real
hardware puts there -- as Michal says, the binman configurations top
out at 37. The case that actually reaches the limit is an EEPROM whose
fields are filled to their declared width, which is a corrupt or
hostile EEPROM rather than a board that legitimately needs more room.
Panicking on that is the right answer; growing the buffer would only
move the threshold. I have no case that needs more than 50.
One thing neither patch touches, which I am happy to send separately if
you want it:
board_local_name = calloc(1, MAX_NAME_LENGTH);
if (!board_info)
return NULL;
The calloc() result is never checked -- the test is on board_info -- so
an allocation failure leaves a NULL destination for the first
strcat()/strlcat(). And that early return leaks board_local_name,
where every other exit goes through the error: label and frees it.
Both look like the same typo.
Thanks,
Sriram
