[PATCH 0/2] board: xilinx: Fix board name overflow and allow board ft_board_setup()

[PATCH 0/2] board: xilinx: Fix board name overflow and allow board ft_board_setup()

[Sriram Sriram]

Two independent fixes to the xilinx vendor common board file, both found
while bringing a downstream Versal NET board up to date with mainline.

Patch 1 fixes a heap buffer overflow in board_name_decode(). The name is
built with strcat() into a 50 byte allocation, and the length check that
is supposed to catch the overflow only runs after the writes. Each EEPROM
descriptor can contribute up to 29 characters, so a base board plus a
carrier card already overruns the buffer with EEPROM-supplied data.

Patch 2 marks ft_board_setup() __weak so that a board using the xilinx
vendor common library can supply its own device tree fixups, the same way
the file already allows board_name_decode(), board_detection(),
soc_detection() and board_rng_seed() to be overridden. Today such a board
fails to link with "multiple definition of ft_board_setup".

Both patches were build tested on xilinx_zynqmp_kria_defconfig, which
enables CONFIG_DTB_RESELECT and CONFIG_OF_BOARD_SETUP and so compiles
both of the changed functions.

Drew Kluemke (1):
  board: xilinx: Use strlcat() in board_name_decode()

Sriram Sriram (1):
  board: xilinx: Make ft_board_setup() weak

 board/xilinx/common/board.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.49.0

[PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Sriram Sriram]

From: Drew Kluemke <ankluemk@microsoft.com>

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.

Use strlcat() bounded to MAX_NAME_LENGTH so the writes are truncated
rather than overflowing the allocation. Truncation leaves a string of
MAX_NAME_LENGTH - 1 characters, so relax the check accordingly;
as written it could never fire once strlcat() is used.

Signed-off-by: Drew Kluemke <ankluemk@microsoft.com>
Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
---
 board/xilinx/common/board.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index f45b879736e..690991bb435 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -590,38 +590,42 @@ char * __maybe_unused __weak board_name_decode(void)
 
 		/* The first string should be soc name */
 		if (!id)
-			strcat(board_local_name, CONFIG_SYS_BOARD);
+			strlcat(board_local_name, CONFIG_SYS_BOARD,
+				MAX_NAME_LENGTH);
 
 		/*
 		 * For two purpose here:
 		 * soc_name- eg: zynqmp-
 		 * and between base board and CC eg: ..revA-sck...
 		 */
-		strcat(board_local_name, "-");
+		strlcat(board_local_name, "-", MAX_NAME_LENGTH);
 
 		if (desc->name[0]) {
 			/* For DT composition name needs to be lowercase */
 			for (i = 0; i < sizeof(desc->name); i++)
 				desc->name[i] = tolower(desc->name[i]);
 
-			strcat(board_local_name, desc->name);
+			strlcat(board_local_name, desc->name,
+				MAX_NAME_LENGTH);
 		}
 		if (desc->revision[0]) {
-			strcat(board_local_name, "-rev");
+			strlcat(board_local_name, "-rev",
+				MAX_NAME_LENGTH);
 
 			/* And revision needs to be uppercase */
 			for (i = 0; i < sizeof(desc->revision); i++)
 				desc->revision[i] = toupper(desc->revision[i]);
 
-			strcat(board_local_name, desc->revision);
+			strlcat(board_local_name, desc->revision,
+				MAX_NAME_LENGTH);
 		}
 	}
 
 	/*
-	 * Longer strings will end up with buffer overflow and potential
-	 * attacks that's why check it
+	 * Longer strings will be truncated by strlcat, check and
+	 * panic if the source data was too long for the buffer.
 	 */
-	if (strlen(board_local_name) >= MAX_NAME_LENGTH)
+	if (strlen(board_local_name) >= MAX_NAME_LENGTH - 1)
 		panic("Board name can't be determined\n");
 
 	if (strlen(board_local_name))
-- 
2.49.0

[PATCH 2/2] board: xilinx: Make ft_board_setup() weak

[Sriram Sriram]

board/xilinx/common/board.c is linked into every board that uses the
xilinx vendor common library, so the strong ft_board_setup() defined
here prevents an individual board from applying its own device tree
fixups: adding ft_board_setup() to the board file fails to link with
"multiple definition of ft_board_setup".

Mark it __weak, which is the mechanism this file already uses for its
other board hooks - board_name_decode(), board_detection(),
soc_detection() and board_rng_seed(). No other weak definition of
ft_board_setup() exists in the tree, so boards without an override keep
linking this implementation and symbol resolution stays unambiguous.

Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
---
 board/xilinx/common/board.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index 690991bb435..0e765cc81b4 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -695,7 +695,7 @@ int embedded_dtb_select(void)
 
 #ifdef CONFIG_OF_BOARD_SETUP
 #define MAX_RAND_SIZE 8
-int ft_board_setup(void *blob, struct bd_info *bd)
+int __weak ft_board_setup(void *blob, struct bd_info *bd)
 {
 	static const struct node_info nodes[] = {
 		{ "arm,pl353-nand-r2p1", MTD_DEV_TYPE_NAND, },
-- 
2.49.0

RE: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Maarten Brock]

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?

Kind regards,
Maarten Brock

> -----Original Message-----
> From: Sriram Sriram <sriramsriram@linux.microsoft.com>
> Sent: Wednesday 9 September 2026 21:21
> To: u-boot@lists.u-boot-project.org
> Cc: Michal Simek <michal.simek@amd.com>; Tom Rini <trini@konsulko.com>; Drew Kluemke <ankluemk@microsoft.com>; Sriram
> Sriram <sriramsriram@linux.microsoft.com>
> Subject: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()
> 
> From: Drew Kluemke <ankluemk@microsoft.com>
> 
> 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.
> 
> Use strlcat() bounded to MAX_NAME_LENGTH so the writes are truncated
> rather than overflowing the allocation. Truncation leaves a string of
> MAX_NAME_LENGTH - 1 characters, so relax the check accordingly;
> as written it could never fire once strlcat() is used.
> 
> Signed-off-by: Drew Kluemke <ankluemk@microsoft.com>
> Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> ---
>  board/xilinx/common/board.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> index f45b879736e..690991bb435 100644
> --- a/board/xilinx/common/board.c
> +++ b/board/xilinx/common/board.c
> @@ -590,38 +590,42 @@ char * __maybe_unused __weak board_name_decode(void)
> 
>  		/* The first string should be soc name */
>  		if (!id)
> -			strcat(board_local_name, CONFIG_SYS_BOARD);
> +			strlcat(board_local_name, CONFIG_SYS_BOARD,
> +				MAX_NAME_LENGTH);
> 
>  		/*
>  		 * For two purpose here:
>  		 * soc_name- eg: zynqmp-
>  		 * and between base board and CC eg: ..revA-sck...
>  		 */
> -		strcat(board_local_name, "-");
> +		strlcat(board_local_name, "-", MAX_NAME_LENGTH);
> 
>  		if (desc->name[0]) {
>  			/* For DT composition name needs to be lowercase */
>  			for (i = 0; i < sizeof(desc->name); i++)
>  				desc->name[i] = tolower(desc->name[i]);
> 
> -			strcat(board_local_name, desc->name);
> +			strlcat(board_local_name, desc->name,
> +				MAX_NAME_LENGTH);
>  		}
>  		if (desc->revision[0]) {
> -			strcat(board_local_name, "-rev");
> +			strlcat(board_local_name, "-rev",
> +				MAX_NAME_LENGTH);
> 
>  			/* And revision needs to be uppercase */
>  			for (i = 0; i < sizeof(desc->revision); i++)
>  				desc->revision[i] = toupper(desc->revision[i]);
> 
> -			strcat(board_local_name, desc->revision);
> +			strlcat(board_local_name, desc->revision,
> +				MAX_NAME_LENGTH);
>  		}
>  	}
> 
>  	/*
> -	 * Longer strings will end up with buffer overflow and potential
> -	 * attacks that's why check it
> +	 * Longer strings will be truncated by strlcat, check and
> +	 * panic if the source data was too long for the buffer.
>  	 */
> -	if (strlen(board_local_name) >= MAX_NAME_LENGTH)
> +	if (strlen(board_local_name) >= MAX_NAME_LENGTH - 1)
>  		panic("Board name can't be determined\n");
> 
>  	if (strlen(board_local_name))
> --
> 2.49.0

Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Michal Simek]

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.

Thanks,
Michal

Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Michal Simek]

On 9/9/26 21:20, Sriram Sriram wrote:
> From: Drew Kluemke <ankluemk@microsoft.com>
> 
> 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.
> 
> Use strlcat() bounded to MAX_NAME_LENGTH so the writes are truncated
> rather than overflowing the allocation. Truncation leaves a string of
> MAX_NAME_LENGTH - 1 characters, so relax the check accordingly;
> as written it could never fire once strlcat() is used.
> 
> Signed-off-by: Drew Kluemke <ankluemk@microsoft.com>
> Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> ---
>   board/xilinx/common/board.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> index f45b879736e..690991bb435 100644
> --- a/board/xilinx/common/board.c
> +++ b/board/xilinx/common/board.c
> @@ -590,38 +590,42 @@ char * __maybe_unused __weak board_name_decode(void)
>   
>   		/* The first string should be soc name */
>   		if (!id)
> -			strcat(board_local_name, CONFIG_SYS_BOARD);
> +			strlcat(board_local_name, CONFIG_SYS_BOARD,
> +				MAX_NAME_LENGTH);

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.

Thanks,
Michal



  Author:     Pranav Tilak <pranav.vinaytilak@amd.com>
  AuthorDate: Wed Sep 9 10:13:09 2026 +0530

      board: xilinx: Fix heap buffer overflow in board_name_decode()

      board_name_decode() builds the board name by repeatedly appending
      to a fixed MAX_NAME_LENGTH (50-byte) buffer with strcat(), and only
      checks for overflow once, after the entire loop over all detected
      nvmemX EEPROM devices has completed. If enough devices are present
      and their name/revision fields are long enough, strcat() can write
      past the end of the buffer before the overflow check is reached,
      resulting in heap memory corruption.

      Fix this by using strlcat() instead, which bounds every write to
      the destination buffer size and prevents overrunning the allocation.
      Use its return value to detect truncation during board name
      construction instead of relying on a single length check after all
      EEPROM devices have been processed.

      Fixes: 52ff1626cf4c ("xilinx: common: Enabling generic function for DT 
reselection")
      Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>

  diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
  index bdccf0732d2a..964817d98e65 100644
  --- a/board/xilinx/common/board.c
  +++ b/board/xilinx/common/board.c
  @@ -571,8 +571,9 @@ int __maybe_unused board_fit_config_name_match(const char 
*name)

   char * __maybe_unused __weak board_name_decode(void)
   {
  -	char *board_local_name;
   	struct xilinx_board_description *desc;
  +	char *board_local_name;
  +	size_t len = 0;
   	int i, id;

   	board_local_name = calloc(1, MAX_NAME_LENGTH);
  @@ -592,39 +593,36 @@ char * __maybe_unused __weak board_name_decode(void)

   		/* The first string should be soc name */
   		if (!id)
  -			strcat(board_local_name, CONFIG_SYS_BOARD);
  +			len = strlcat(board_local_name, CONFIG_SYS_BOARD, MAX_NAME_LENGTH);

   		/*
   		 * For two purpose here:
   		 * soc_name- eg: zynqmp-
   		 * and between base board and CC eg: ..revA-sck...
   		 */
  -		strcat(board_local_name, "-");
  +		len = strlcat(board_local_name, "-", MAX_NAME_LENGTH);

   		if (desc->name[0]) {
   			/* For DT composition name needs to be lowercase */
   			for (i = 0; i < sizeof(desc->name); i++)
   				desc->name[i] = tolower(desc->name[i]);

  -			strcat(board_local_name, desc->name);
  +			len = strlcat(board_local_name, desc->name, MAX_NAME_LENGTH);
   		}
   		if (desc->revision[0]) {
  -			strcat(board_local_name, "-rev");
  +			len = strlcat(board_local_name, "-rev", MAX_NAME_LENGTH);

   			/* And revision needs to be uppercase */
   			for (i = 0; i < sizeof(desc->revision); i++)
   				desc->revision[i] = toupper(desc->revision[i]);

  -			strcat(board_local_name, desc->revision);
  +			len = strlcat(board_local_name, desc->revision, MAX_NAME_LENGTH);
   		}
  -	}

  -	/*
  -	 * Longer strings will end up with buffer overflow and potential
  -	 * attacks that's why check it
  -	 */
  -	if (strlen(board_local_name) >= MAX_NAME_LENGTH)
  -		panic("Board name can't be determined\n");
  +		/* Reject truncated board names. */
  +		if (len >= MAX_NAME_LENGTH)
  +			panic("Board name can't be determined\n");
  +	}

   	if (strlen(board_local_name))
   		return board_local_name;

RE: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Maarten Brock]

Hello Michal,

> -----Original Message-----
> From: Michal Simek <michal.simek@amd.com>
> 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 <michal.simek@amd.com>
> Sent: Thursday 10 September 2026 10:04
> 
> On 9/9/26 21:20, Sriram Sriram wrote:
> > From: Drew Kluemke <ankluemk@microsoft.com>
> >
> > 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.

> > Use strlcat() bounded to MAX_NAME_LENGTH so the writes are truncated
> > rather than overflowing the allocation. Truncation leaves a string of
> > MAX_NAME_LENGTH - 1 characters, so relax the check accordingly;
> > as written it could never fire once strlcat() is used.
> >
> > Signed-off-by: Drew Kluemke <ankluemk@microsoft.com>
> > Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> > ---
> >   board/xilinx/common/board.c | 20 ++++++++++++--------
> >   1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
> > index f45b879736e..690991bb435 100644
> > --- a/board/xilinx/common/board.c
> > +++ b/board/xilinx/common/board.c
> > @@ -590,38 +590,42 @@ char * __maybe_unused __weak board_name_decode(void)
> >
> >   		/* The first string should be soc name */
> >   		if (!id)
> > -			strcat(board_local_name, CONFIG_SYS_BOARD);
> > +			strlcat(board_local_name, CONFIG_SYS_BOARD,
> > +				MAX_NAME_LENGTH);
> 
> 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.
> 
> Thanks,
> Michal
> 
> 
> 
>   Author:     Pranav Tilak <pranav.vinaytilak@amd.com>
>   AuthorDate: Wed Sep 9 10:13:09 2026 +0530
> 
>       board: xilinx: Fix heap buffer overflow in board_name_decode()
> 
>       board_name_decode() builds the board name by repeatedly appending
>       to a fixed MAX_NAME_LENGTH (50-byte) buffer with strcat(), and only
>       checks for overflow once, after the entire loop over all detected
>       nvmemX EEPROM devices has completed. If enough devices are present
>       and their name/revision fields are long enough, strcat() can write
>       past the end of the buffer before the overflow check is reached,
>       resulting in heap memory corruption.
> 
>       Fix this by using strlcat() instead, which bounds every write to
>       the destination buffer size and prevents overrunning the allocation.
>       Use its return value to detect truncation during board name
>       construction instead of relying on a single length check after all
>       EEPROM devices have been processed.
> 
>       Fixes: 52ff1626cf4c ("xilinx: common: Enabling generic function for DT
> reselection")
>       Signed-off-by: Pranav Tilak <pranav.vinaytilak@amd.com>
> 
>   diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
>   index bdccf0732d2a..964817d98e65 100644
>   --- a/board/xilinx/common/board.c
>   +++ b/board/xilinx/common/board.c
>   @@ -571,8 +571,9 @@ int __maybe_unused board_fit_config_name_match(const char
> *name)
> 
>    char * __maybe_unused __weak board_name_decode(void)
>    {
>   -	char *board_local_name;
>    	struct xilinx_board_description *desc;
>   +	char *board_local_name;
>   +	size_t len = 0;
>    	int i, id;
> 
>    	board_local_name = calloc(1, MAX_NAME_LENGTH);
>   @@ -592,39 +593,36 @@ char * __maybe_unused __weak board_name_decode(void)
> 
>    		/* The first string should be soc name */
>    		if (!id)
>   -			strcat(board_local_name, CONFIG_SYS_BOARD);
>   +			len = strlcat(board_local_name, CONFIG_SYS_BOARD, MAX_NAME_LENGTH);
> 
>    		/*
>    		 * For two purpose here:
>    		 * soc_name- eg: zynqmp-
>    		 * and between base board and CC eg: ..revA-sck...
>    		 */
>   -		strcat(board_local_name, "-");
>   +		len = strlcat(board_local_name, "-", MAX_NAME_LENGTH);
> 
>    		if (desc->name[0]) {
>    			/* For DT composition name needs to be lowercase */
>    			for (i = 0; i < sizeof(desc->name); i++)
>    				desc->name[i] = tolower(desc->name[i]);
> 
>   -			strcat(board_local_name, desc->name);
>   +			len = strlcat(board_local_name, desc->name, MAX_NAME_LENGTH);
>    		}
>    		if (desc->revision[0]) {
>   -			strcat(board_local_name, "-rev");
>   +			len = strlcat(board_local_name, "-rev", MAX_NAME_LENGTH);
> 
>    			/* And revision needs to be uppercase */
>    			for (i = 0; i < sizeof(desc->revision); i++)
>    				desc->revision[i] = toupper(desc->revision[i]);
> 
>   -			strcat(board_local_name, desc->revision);
>   +			len = strlcat(board_local_name, desc->revision, MAX_NAME_LENGTH);
>    		}
>   -	}
> 
>   -	/*
>   -	 * Longer strings will end up with buffer overflow and potential
>   -	 * attacks that's why check it
>   -	 */
>   -	if (strlen(board_local_name) >= MAX_NAME_LENGTH)
>   -		panic("Board name can't be determined\n");
>   +		/* Reject truncated board names. */
>   +		if (len >= MAX_NAME_LENGTH)
>   +			panic("Board name can't be determined\n");
>   +	}
> 
>    	if (strlen(board_local_name))
>    		return board_local_name;
> 

I would prefer the patch from Pranav Tilak which doesn't need an extra strlen() call.

Kind Regards,
Maarten

Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Michal Simek]

On 9/10/26 11:30, Maarten Brock wrote:
> Hello Michal,
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@amd.com>
>> 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 <michal.simek@amd.com>
>> Sent: Thursday 10 September 2026 10:04
>>
>> On 9/9/26 21:20, Sriram Sriram wrote:
>>> From: Drew Kluemke <ankluemk@microsoft.com>
>>>
>>> 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

Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Sriram Sriram]

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 <michal.simek@amd.com>
> > > 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 <michal.simek@amd.com>
> > > Sent: Thursday 10 September 2026 10:04
> > > 
> > > On 9/9/26 21:20, Sriram Sriram wrote:
> > > > From: Drew Kluemke <ankluemk@microsoft.com>
> > > > 
> > > > 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

Re: [PATCH 1/2] board: xilinx: Use strlcat() in board_name_decode()

[Michal Simek]

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

Yes. Please send a patch for it with also 2/2 from this series.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP/Versal ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal/Versal NET SoCs
TF-A maintainer - Xilinx ZynqMP/Versal/Versal NET SoCs

[PATCH v2 0/2] board: xilinx: board_name_decode() allocation fix and weak ft_board_setup()

[Sriram Sriram]

v1 1/2 ("board: xilinx: Use strlcat() in board_name_decode()") is dropped
from this series in favour of Pranav Tilak's patch, which fixes the same
heap overflow without the extra strlen().

It is replaced here by a separate defect in the same function that
neither version of the strlcat() conversion touches: the calloc() result
is never checked (the test is on board_info), and that early return is
the one exit path that does not free board_local_name.

2/2 is unchanged from v1 and is resent per Michal's request.

Sriram Sriram (2):
  board: xilinx: Check calloc() result in board_name_decode()
  board: xilinx: Make ft_board_setup() weak

 board/xilinx/common/board.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] board: xilinx: Check calloc() result in board_name_decode()

[Sriram Sriram]

The allocation of board_local_name is followed by a test on board_info
instead of on the newly allocated pointer, so an allocation failure is
never detected and the first strcat() writes through a NULL pointer.

The same test is also placed after the allocation, so returning early
because board_info is NULL leaks the buffer, while every other exit from
the function goes through the error: label and frees it.

Test board_info before allocating anything and check the calloc() result
on its own.

Fixes: 52ff1626cf4c ("xilinx: common: Enabling generic function for DT reselection")
Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
---

Changes in v2:
- New patch. Replaces v1 1/2 ("board: xilinx: Use strlcat() in
  board_name_decode()"), which is dropped in favour of Pranav Tilak's
  patch for the same heap overflow.

 board/xilinx/common/board.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index f45b879736e..9ea5f46c9a8 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -573,10 +573,13 @@ char * __maybe_unused __weak board_name_decode(void)
 	struct xilinx_board_description *desc;
 	int i, id;
 
-	board_local_name = calloc(1, MAX_NAME_LENGTH);
 	if (!board_info)
 		return NULL;
 
+	board_local_name = calloc(1, MAX_NAME_LENGTH);
+	if (!board_local_name)
+		return NULL;
+
 	for (id = 0; id <= highest_id; id++) {
 		desc = &board_info[id];
 
-- 
2.49.0

[PATCH v2 2/2] board: xilinx: Make ft_board_setup() weak

[Sriram Sriram]

board/xilinx/common/board.c is linked into every board that uses the
xilinx vendor common library, so the strong ft_board_setup() defined
here prevents an individual board from applying its own device tree
fixups: adding ft_board_setup() to the board file fails to link with
"multiple definition of ft_board_setup".

Mark it __weak, which is the mechanism this file already uses for its
other board hooks - board_name_decode(), board_detection(),
soc_detection() and board_rng_seed(). No other weak definition of
ft_board_setup() exists in the tree, so boards without an override keep
linking this implementation and symbol resolution stays unambiguous.

Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
---

Changes in v2:
- No change.

 board/xilinx/common/board.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c
index 9ea5f46c9a8..497bbe09bc8 100644
--- a/board/xilinx/common/board.c
+++ b/board/xilinx/common/board.c
@@ -694,7 +694,7 @@ int embedded_dtb_select(void)
 
 #ifdef CONFIG_OF_BOARD_SETUP
 #define MAX_RAND_SIZE 8
-int ft_board_setup(void *blob, struct bd_info *bd)
+int __weak ft_board_setup(void *blob, struct bd_info *bd)
 {
 	static const struct node_info nodes[] = {
 		{ "arm,pl353-nand-r2p1", MTD_DEV_TYPE_NAND, },
-- 
2.49.0

Re: [PATCH v2 0/2] board: xilinx: board_name_decode() allocation fix and weak ft_board_setup()

[Michal Simek]

On 9/16/26 19:36, Sriram Sriram wrote:
> v1 1/2 ("board: xilinx: Use strlcat() in board_name_decode()") is dropped
> from this series in favour of Pranav Tilak's patch, which fixes the same
> heap overflow without the extra strlen().
> 
> It is replaced here by a separate defect in the same function that
> neither version of the strlcat() conversion touches: the calloc() result
> is never checked (the test is on board_info), and that early return is
> the one exit path that does not free board_local_name.
> 
> 2/2 is unchanged from v1 and is resent per Michal's request.
> 
> Sriram Sriram (2):
>    board: xilinx: Check calloc() result in board_name_decode()
>    board: xilinx: Make ft_board_setup() weak
> 
>   board/xilinx/common/board.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 

Applied.
M

Re: [PATCH v2 0/2] board: xilinx: board_name_decode() allocation fix and weak ft_board_setup()

[Sriram Sriram]

On Fri, Sep 25, 2026 at 03:51:57PM +0200, Michal Simek wrote:
> 
> 
> On 9/16/26 19:36, Sriram Sriram wrote:
> > v1 1/2 ("board: xilinx: Use strlcat() in board_name_decode()") is dropped
> > from this series in favour of Pranav Tilak's patch, which fixes the same
> > heap overflow without the extra strlen().
> > 
> > It is replaced here by a separate defect in the same function that
> > neither version of the strlcat() conversion touches: the calloc() result
> > is never checked (the test is on board_info), and that early return is
> > the one exit path that does not free board_local_name.
> > 
> > 2/2 is unchanged from v1 and is resent per Michal's request.
> > 
> > Sriram Sriram (2):
> >    board: xilinx: Check calloc() result in board_name_decode()
> >    board: xilinx: Make ft_board_setup() weak
> > 
> >   board/xilinx/common/board.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> 
> Applied.
> M

Great! Thank you.

Regards,
Sriram