[PATCH 0/4] test: uncover NULL dereferences in logging

[PATCH 0/4] test: uncover NULL dereferences in logging

[Heinrich Schuchardt]

Setting CONFIG_LOG_MAX_LEVEL=9 in sandbox_defconfig exposed to NULL
dereferences and bugs in the log tests.

log_debug() statements are only executed for CONFIG_LOG_MAX_LEVEL > 6.
We should have a defconfig that exposes problems in these statements.

Heinrich Schuchardt (4):
  dm: avoid NULL dereference in add_item()
  test: fix log tests
  dm: avoid NULL dereference in lists_bind_fdt()
  configs: sandbox_defconfig: CONFIG_LOG_MAX_LEVEL=9

 configs/sandbox_defconfig |  2 ++
 drivers/core/acpi.c       |  4 ++--
 drivers/core/lists.c      |  7 ++++---
 test/log/log_test.c       | 11 +++++++----
 4 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.36.1

[PATCH 1/4] dm: avoid NULL dereference in add_item()

[Heinrich Schuchardt]

acpi_add_other_item() passes dev = NULL. Instead of dev->name write the
string "other" to the debug log:

    ACPI: Writing ACPI tables at 1fd3000
    0base: writing table '<NULL>'
    * other: Added type 3, 0000000011fd4000, size 240
    1facs: writing table 'FACS'
    * other: Added type 3, 0000000011fd4240, size 40
    5csrt: writing table 'CSRT'
    * other: Added type 3, 0000000011fd4280, size 30

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
Resent
---
 drivers/core/acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/core/acpi.c b/drivers/core/acpi.c
index 0df58dbc0d..8457733edb 100644
--- a/drivers/core/acpi.c
+++ b/drivers/core/acpi.c
@@ -159,8 +159,8 @@ static int add_item(struct acpi_ctx *ctx, struct udevice *dev,
 		memcpy(item->buf, start, item->size);
 	}
 	item_count++;
-	log_debug("* %s: Added type %d, %p, size %x\n", dev->name, type, start,
-		  item->size);
+	log_debug("* %s: Added type %d, %p, size %x\n",
+		  dev ? dev->name : "other", type, start, item->size);
 
 	return 0;
 }
-- 
2.36.1

[PATCH 2/4] test: fix log tests

[Heinrich Schuchardt]

Consider CONFIG_LOG_MAX_LEVEL and gd->default_log_level in

* do_log_test_helpers()
* log_test_dropped()
* log_test_level_deny()

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 test/log/log_test.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/test/log/log_test.c b/test/log/log_test.c
index db7170f304..c5abff80d1 100644
--- a/test/log/log_test.c
+++ b/test/log/log_test.c
@@ -277,7 +277,7 @@ int do_log_test_helpers(struct unit_test_state *uts)
 	log_content("level %d\n", LOGL_DEBUG_CONTENT);
 	log_io("level %d\n", LOGL_DEBUG_IO);
 
-	for (i = LOGL_EMERG; i <= _LOG_MAX_LEVEL; i++)
+	for (i = LOGL_EMERG; i <= gd->default_log_level; i++)
 		ut_assert_nextline("%*s() level %d", CONFIG_LOGF_FUNC_PAD,
 				   __func__, i);
 	ut_assert_console_end();
@@ -381,7 +381,8 @@ int log_test_level_deny(struct unit_test_state *uts)
 	ut_assertok(console_record_reset_enable());
 	log_run();
 	check_log_entries_flags_levels(EXPECT_LOG | EXPECT_DIRECT | EXPECT_FORCE,
-				       LOGL_WARNING + 1, _LOG_MAX_LEVEL);
+				       LOGL_WARNING + 1,
+				       min(gd->default_log_level, LOGL_INFO));
 
 	ut_assertok(log_remove_filter("console", filt1));
 	ut_assertok(log_remove_filter("console", filt2));
@@ -420,9 +421,11 @@ int log_test_dropped(struct unit_test_state *uts)
 	gd->log_drop_count = 0;
 
 	ut_assertok(console_record_reset_enable());
-	log_run();
 
-	ut_asserteq(gd->log_drop_count, 3 * (LOGL_COUNT - LOGL_FIRST - 1));
+	log_run();
+	ut_asserteq(2 * (LOGL_COUNT - LOGL_FIRST) +
+		    _LOG_MAX_LEVEL - LOGL_FIRST + 1,
+		    gd->log_drop_count);
 	check_log_entries_flags_levels(EXPECT_DEBUG, LOGL_FIRST, CONFIG_LOG_DEFAULT_LEVEL);
 
 	gd->flags |= GD_FLG_LOG_READY;
-- 
2.36.1

Re: [PATCH 2/4] test: fix log tests

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Consider CONFIG_LOG_MAX_LEVEL and gd->default_log_level in
>
> * do_log_test_helpers()
> * log_test_dropped()
> * log_test_level_deny()
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  test/log/log_test.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/4] test: fix log tests

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Consider CONFIG_LOG_MAX_LEVEL and gd->default_log_level in
>
> * do_log_test_helpers()
> * log_test_dropped()
> * log_test_level_deny()
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  test/log/log_test.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm, thanks!

[PATCH 3/4] dm: avoid NULL dereference in lists_bind_fdt()

[Heinrich Schuchardt]

If parameter drv of lists_bind_fdt() is specified, we want to bind to this
specific driver even if its field of_match is NULL.

If entry->of_match is NULL, we should not dereference it in a debug
statement.

Fixes: d3e773613b6d ("dm: core: Use U-Boot logging instead of pr_debug()")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/core/lists.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 22ccd9faaa..93514a744d 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -241,9 +241,10 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 			}
 		}
 
-		log_debug("   - found match at '%s': '%s' matches '%s'\n",
-			  entry->name, entry->of_match->compatible,
-			  id->compatible);
+		if (entry->of_match)
+			log_debug("   - found match at '%s': '%s' matches '%s'\n",
+				  entry->name, entry->of_match->compatible,
+				  id->compatible);
 		ret = device_bind_with_driver_data(parent, entry, name,
 						   id->data, node, &dev);
 		if (ret == -ENODEV) {
-- 
2.36.1

Re: [PATCH 3/4] dm: avoid NULL dereference in lists_bind_fdt()

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> If parameter drv of lists_bind_fdt() is specified, we want to bind to this
> specific driver even if its field of_match is NULL.
>
> If entry->of_match is NULL, we should not dereference it in a debug
> statement.
>
> Fixes: d3e773613b6d ("dm: core: Use U-Boot logging instead of pr_debug()")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/core/lists.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/4] dm: avoid NULL dereference in lists_bind_fdt()

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> If parameter drv of lists_bind_fdt() is specified, we want to bind to this
> specific driver even if its field of_match is NULL.
>
> If entry->of_match is NULL, we should not dereference it in a debug
> statement.
>
> Fixes: d3e773613b6d ("dm: core: Use U-Boot logging instead of pr_debug()")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/core/lists.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm, thanks!

[PATCH 4/4] configs: sandbox_defconfig: CONFIG_LOG_MAX_LEVEL=9

[Heinrich Schuchardt]

Without setting CONFIG_LOG_MAX_LEVEL to a value above 6 we will not detect
NULL dereferences and other errors in log_debug() calls.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
Resent
---
 configs/sandbox_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index c509a924e6..4b87d96c76 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -31,6 +31,8 @@ CONFIG_CONSOLE_RECORD=y
 CONFIG_CONSOLE_RECORD_OUT_SIZE=0x6000
 CONFIG_PRE_CONSOLE_BUFFER=y
 CONFIG_LOG=y
+CONFIG_LOG_MAX_LEVEL=9
+CONFIG_LOG_DEFAULT_LEVEL=6
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_STACKPROTECTOR=y
 CONFIG_ANDROID_AB=y
-- 
2.36.1

Re: [PATCH 4/4] configs: sandbox_defconfig: CONFIG_LOG_MAX_LEVEL=9

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Without setting CONFIG_LOG_MAX_LEVEL to a value above 6 we will not detect
> NULL dereferences and other errors in log_debug() calls.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> Resent
> ---
>  configs/sandbox_defconfig | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 4/4] configs: sandbox_defconfig: CONFIG_LOG_MAX_LEVEL=9

[Simon Glass]

On Mon, 11 Jul 2022 at 00:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Without setting CONFIG_LOG_MAX_LEVEL to a value above 6 we will not detect
> NULL dereferences and other errors in log_debug() calls.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> Resent
> ---
>  configs/sandbox_defconfig | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm, thanks!