Re: [PATCH] drm/amd/display: use proper context for logging

[Jiri Slaby]

On 22. 07. 26, 8:51, sashiko-...@kernel.org wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:
Pre-existing issues:
- [High] NULL pointer dereference when logging errors in 
`dce112_get_pix_clk_dividers` and `dcn3_get_pix_clk_dividers`.
--

--- Patch [1]: [PATCH] drm/amd/display: use proper context for logging ---
commit 67013b697b017867bb1c1e61f13b863b96364c5e
Author: Jiri Slaby (SUSE) <jirisl...@kernel.org>

drm/amd/display: use proper context for logging

This commit fixes a NULL pointer dereference in get_ss_info_from_atombios()
by temporarily redefining DC_LOGGER to use clk_src->base.ctx->logger. The
calc_pll_cs->ctx context is uninitialized at that time since it is only set
later by calc_pll_max_vco_construct().

Closes: https://bugzilla.suse.com/show_bug.cgi?id=1271175
Closes: 
https://lore.kernel.org/all/a9ee54e6-2413-4156-9bde-d528ae3c6...@kernel.org/
Fixes: 1296423bf23c ("drm/amd/display: define DC_LOGGER for logger")

diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c 
b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
index ecb8493ec5233..d3f7aa853e3a6 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
@@ -1513,6 +1513,10 @@ static const struct clock_source_funcs 
dce110_clk_src_funcs = {
        .get_dp_dto_frequency_100hz = get_dp_dto_frequency_100hz
  };
  
+/* calc_pll_cs->ctx is set only after get_ss_info_from_atombios(), unlike clk_src->base */
+#pragma push_macro("DC_LOGGER")
+#undef DC_LOGGER
+#define DC_LOGGER CTX->logger

[Severity: High]
This is a pre-existing issue, but does a similar NULL pointer dereference
exist in dce112_get_pix_clk_dividers() and dcn3_get_pix_clk_dividers()
during their error paths?

It appears dce112_clk_src_construct() does not call
calc_pll_max_vco_construct(), leaving calc_pll_cs->ctx as NULL. If invalid
parameters are passed (like requested_pix_clk_100hz == 0), the error path
invokes DC_LOG_ERROR() which dereferences calc_pll_cs->ctx->logger:

drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c:dce112_get_pix_clk_dividers()
 {
         DC_LOGGER_INIT();

         if (pix_clk_params == NULL || pll_settings == NULL
                         || pix_clk_params->requested_pix_clk_100hz == 0) {
                 DC_LOG_ERROR(
                         "%s: Invalid parameters!!\n", __func__);
                 return (uint32_t)-1;
         }
         ...
}

A similar error path exists in dcn3_get_pix_clk_dividers().

Since DC_LOG_ERROR() relies on the same uninitialized context pointer,
could this trigger a crash similar to the one being fixed here if these
functions encounter invalid parameters?

Occurs likely to me. Someone involved in the driver has to look as the 
logic is really intertwined.

thanks,
--
js
suse labs