[PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Marek Vasut]

The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
The current NULL pointer check happens too late, after the eip was
accessed in variable assignment. Reorder the two, so the NULL pointer
check happens first, and any access second, otherwise the access may
trigger a hang or other undefined behavior.

Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 board/dhelectronics/common/dh_common.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
index 9f8e5754d8c..aeabd617374 100644
--- a/board/dhelectronics/common/dh_common.c
+++ b/board/dhelectronics/common/dh_common.c
@@ -131,14 +131,17 @@ int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
 int dh_get_value_from_eeprom_buffer(enum eip_request_values request, u8 *data, int data_len,
 				    struct eeprom_id_page *eip)
 {
-	const char fin_chr = (eip->pl.item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
-			     DH_ITEM_PREFIX_FIN_FLASHED_CHR : DH_ITEM_PREFIX_FIN_HALF_CHR;
-	const u8 soc_coded = eip->pl.item_prefix & 0xf;
+	char fin_chr;
+	u8 soc_coded;
 	char soc_chr;
 
 	if (!eip)
 		return -EINVAL;
 
+	fin_chr = (eip->pl.item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
+		  DH_ITEM_PREFIX_FIN_FLASHED_CHR : DH_ITEM_PREFIX_FIN_HALF_CHR;
+	soc_coded = eip->pl.item_prefix & 0xf;
+
 	/* Copy requested data */
 	switch (request) {
 	case DH_MAC0:
-- 
2.50.1

[PATCH 2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()

[Marek Vasut]

The isprint() checks printability across all 256 characters, some of the
upper 128 characters are printable and produce artifacts on UART. Call
isascii() first to only consider the bottom 7bit ASCII characters as
printable, and then check their printability using isprint(). This fixes
a rare misprint in case the ID page content is uninitialized or corrupted.

Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 board/dhelectronics/common/dh_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/board/dhelectronics/common/dh_common.c b/board/dhelectronics/common/dh_common.c
index aeabd617374..e7ee23aa8ce 100644
--- a/board/dhelectronics/common/dh_common.c
+++ b/board/dhelectronics/common/dh_common.c
@@ -88,9 +88,9 @@ int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
 	/* Validate header ID */
 	if (eip->hdr.id[0] != 'D' || eip->hdr.id[1] != 'H' || eip->hdr.id[2] != 'E') {
 		printf("%s: Error validating header ID! (got %c%c%c (0x%02x 0x%02x 0x%02x) != expected DHE)\n",
-		       __func__, isprint(eip->hdr.id[0]) ? eip->hdr.id[0] : '.',
-		       isprint(eip->hdr.id[1]) ? eip->hdr.id[1] : '.',
-		       isprint(eip->hdr.id[2]) ? eip->hdr.id[2] : '.',
+		       __func__, (isascii(eip->hdr.id[0]) && isprint(eip->hdr.id[0])) ? eip->hdr.id[0] : '.',
+		       (isascii(eip->hdr.id[1]) && isprint(eip->hdr.id[1])) ? eip->hdr.id[1] : '.',
+		       (isascii(eip->hdr.id[2]) && isprint(eip->hdr.id[2])) ? eip->hdr.id[2] : '.',
 		       eip->hdr.id[0], eip->hdr.id[1], eip->hdr.id[2]);
 		return -EINVAL;
 	}
-- 
2.50.1

RE: [PATCH 2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()

[Christoph Niedermaier]

From: Marek Vasut <marek.vasut@mailbox.org>
Sent: Sunday, September 7, 2025 3:01 AM
> The isprint() checks printability across all 256 characters, some of the
> upper 128 characters are printable and produce artifacts on UART. Call
> isascii() first to only consider the bottom 7bit ASCII characters as
> printable, and then check their printability using isprint(). This fixes
> a rare misprint in case the ID page content is uninitialized or corrupted.
> 
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  board/dhelectronics/common/dh_common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/board/dhelectronics/common/dh_common.c
> b/board/dhelectronics/common/dh_common.c
> index aeabd617374..e7ee23aa8ce 100644
> --- a/board/dhelectronics/common/dh_common.c
> +++ b/board/dhelectronics/common/dh_common.c
> @@ -88,9 +88,9 @@ int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
>         /* Validate header ID */
>         if (eip->hdr.id[0] != 'D' || eip->hdr.id[1] != 'H' || eip->hdr.id[2] != 'E') {
>                 printf("%s: Error validating header ID! (got %c%c%c (0x%02x 0x%02x 0x%02x)
> != expected DHE)\n",
> -                      __func__, isprint(eip->hdr.id[0]) ? eip->hdr.id[0] : '.',
> -                      isprint(eip->hdr.id[1]) ? eip->hdr.id[1] : '.',
> -                      isprint(eip->hdr.id[2]) ? eip->hdr.id[2] : '.',
> +                      __func__, (isascii(eip->hdr.id[0]) && isprint(eip->hdr.id[0])) ?
> eip->hdr.id[0] : '.',
> +                      (isascii(eip->hdr.id[1]) && isprint(eip->hdr.id[1])) ? eip-
> >hdr.id[1] : '.',
> +                      (isascii(eip->hdr.id[2]) && isprint(eip->hdr.id[2])) ? eip-
> >hdr.id[2] : '.',
>                        eip->hdr.id[0], eip->hdr.id[1], eip->hdr.id[2]);
>                 return -EINVAL;
>         }

Reviewed-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>

RE: [PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Christoph Niedermaier]

From: Marek Vasut <marek.vasut@mailbox.org>
Sent: Sunday, September 7, 2025 3:01 AM
> The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
> The current NULL pointer check happens too late, after the eip was
> accessed in variable assignment. Reorder the two, so the NULL pointer
> check happens first, and any access second, otherwise the access may
> trigger a hang or other undefined behavior.
> 
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  board/dhelectronics/common/dh_common.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/board/dhelectronics/common/dh_common.c
> b/board/dhelectronics/common/dh_common.c
> index 9f8e5754d8c..aeabd617374 100644
> --- a/board/dhelectronics/common/dh_common.c
> +++ b/board/dhelectronics/common/dh_common.c
> @@ -131,14 +131,17 @@ int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias)
>  int dh_get_value_from_eeprom_buffer(enum eip_request_values request, u8 *data, int
> data_len,
>                                     struct eeprom_id_page *eip)
>  {
> -       const char fin_chr = (eip->pl.item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
> -                            DH_ITEM_PREFIX_FIN_FLASHED_CHR : DH_ITEM_PREFIX_FIN_HALF_CHR;
> -       const u8 soc_coded = eip->pl.item_prefix & 0xf;
> +       char fin_chr;
> +       u8 soc_coded;
>         char soc_chr;
> 
>         if (!eip)
>                 return -EINVAL;
> 
> +       fin_chr = (eip->pl.item_prefix & DH_ITEM_PREFIX_FIN_BIT) ?
> +                 DH_ITEM_PREFIX_FIN_FLASHED_CHR : DH_ITEM_PREFIX_FIN_HALF_CHR;
> +       soc_coded = eip->pl.item_prefix & 0xf;
> +
>         /* Copy requested data */
>         switch (request) {
>         case DH_MAC0:

Reviewed-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>

Re: [PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Tom Rini]

On Sun, 07 Sep 2025 03:00:46 +0200, Marek Vasut wrote:

> The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
> The current NULL pointer check happens too late, after the eip was
> accessed in variable assignment. Reorder the two, so the NULL pointer
> check happens first, and any access second, otherwise the access may
> trigger a hang or other undefined behavior.
> 
> 
> [...]

Applied to u-boot/next, thanks!

[1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()
      commit: 1c735620e19e2ae07705cc38da1552ee6a696ff0
[2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()
      commit: fd396316432ad4a0f2998ea9eee9720be0e5d5f8
-- 
Tom

Re: [PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Marek Vasut]

On 9/17/25 3:47 PM, Tom Rini wrote:
> On Sun, 07 Sep 2025 03:00:46 +0200, Marek Vasut wrote:
> 
>> The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
>> The current NULL pointer check happens too late, after the eip was
>> accessed in variable assignment. Reorder the two, so the NULL pointer
>> check happens first, and any access second, otherwise the access may
>> trigger a hang or other undefined behavior.
>>
>>
>> [...]
> 
> Applied to u-boot/next, thanks!
> 
> [1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()
>        commit: 1c735620e19e2ae07705cc38da1552ee6a696ff0
> [2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()
>        commit: fd396316432ad4a0f2998ea9eee9720be0e5d5f8

These are bugfixes for master.

Re: [PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Tom Rini]

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

On Wed, Sep 17, 2025 at 03:54:24PM +0200, Marek Vasut wrote:
> On 9/17/25 3:47 PM, Tom Rini wrote:
> > On Sun, 07 Sep 2025 03:00:46 +0200, Marek Vasut wrote:
> > 
> > > The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
> > > The current NULL pointer check happens too late, after the eip was
> > > accessed in variable assignment. Reorder the two, so the NULL pointer
> > > check happens first, and any access second, otherwise the access may
> > > trigger a hang or other undefined behavior.
> > > 
> > > 
> > > [...]
> > 
> > Applied to u-boot/next, thanks!
> > 
> > [1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()
> >        commit: 1c735620e19e2ae07705cc38da1552ee6a696ff0
> > [2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()
> >        commit: fd396316432ad4a0f2998ea9eee9720be0e5d5f8
> 
> These are bugfixes for master.

OK, I'll apply it there too now, but please note that cover letters /
fixes tags help make it clearer when a fix is important enough to come
in to master later in the cycle vs wait for the next release.

-- 
Tom

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

Re: [PATCH 1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()

[Tom Rini]

On Sun, 07 Sep 2025 03:00:46 +0200, Marek Vasut wrote:

> The eip pointer in dh_get_value_from_eeprom_buffer() might be NULL.
> The current NULL pointer check happens too late, after the eip was
> accessed in variable assignment. Reorder the two, so the NULL pointer
> check happens first, and any access second, otherwise the access may
> trigger a hang or other undefined behavior.
> 
> 
> [...]

Applied to u-boot/master, thanks!

[1/2] board: dhelectronics: Check pointer before access in dh_get_value_from_eeprom_buffer()
      commit: 5634cf1afcf17fda0136693f02a843aa6b938b2b
[2/2] board: dhelectronics: Use isascii() before isprint() in dh_read_eeprom_id_page()
      commit: 7cd9d2db2627ce65d3c03e4422607b7a86093e8a
-- 
Tom