[PATCH] drm/amd/display: Fix missing NULL check in dcn21_set_backlight_level()
On the off chance 'panel_cntl' ends up being not properly initialized,
dcn21_set_backlight_level() may hit NULL pointer dereference while
changing embedded panel backlight levels.
Prevent this issue by using some of the existing checks for the
similar purpose. At the same time clean up redundant tests for
NULL in 'abm'.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 6f0ef80a00ad ("drm/amd/display: Fix ABM pipe/backlight issues when
change backlight")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
index 8e88dcaf88f5..2b1b580541a8 100644
--- a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
@@ -247,7 +247,7 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
if (abm != NULL) {
uint32_t otg_inst = pipe_ctx->stream_res.tg->inst;
- if (abm && panel_cntl) {
+ if (panel_cntl) {
if (abm->funcs && abm->funcs->set_pipe_ex) {
abm->funcs->set_pipe_ex(abm,
otg_inst,
@@ -261,15 +261,16 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
panel_cntl->inst,
panel_cntl->pwrseq_inst);
}
+
+ if (abm->funcs && abm->funcs->set_backlight_level_pwm)
+ abm->funcs->set_backlight_level_pwm(abm,
backlight_pwm_u16_16,
+ frame_ramp, 0, panel_cntl->inst);
+ else
+ dmub_abm_set_backlight(dc,
backlight_pwm_u16_16, frame_ramp,
+ panel_cntl->inst);
}
}
- if (abm && abm->funcs && abm->funcs->set_backlight_level_pwm)
- abm->funcs->set_backlight_level_pwm(abm, backlight_pwm_u16_16,
- frame_ramp, 0, panel_cntl->inst);
- else
- dmub_abm_set_backlight(dc, backlight_pwm_u16_16, frame_ramp,
panel_cntl->inst);
-
return true;
}
Re: [PATCH] drm/i915/gt: Prevent possible NULL dereference in __caps_show()
Hello,
On 2/7/24 01:16, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 06/02/2024 16:45, Nikita Zhandarovich wrote:
>> After falling through the switch statement to default case 'repr' is
>> initialized with NULL, which will lead to incorrect dereference of
>> '!repr[n]' in the following loop.
>>
>> Fix it with the help of an additional check for NULL.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 4ec76dbeb62b ("drm/i915/gt: Expose engine properties via sysfs")
>> Signed-off-by: Nikita Zhandarovich
>> ---
>> P.S. The NULL-deref problem might be dealt with this way but I am
>> not certain that the rest of the __caps_show() behaviour remains
>> correct if we end up in default case. For instance, as far as I
>> can tell, buf might turn out to be w/o '\0'. I could use some
>> direction if this has to be addressed as well.
>>
>> drivers/gpu/drm/i915/gt/sysfs_engines.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 021f51d9b456..6b130b732867 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -105,7 +105,7 @@ __caps_show(struct intel_engine_cs *engine,
>> len = 0;
>> for_each_set_bit(n, , show_unknown ? BITS_PER_LONG : count) {
>> - if (n >= count || !repr[n]) {
>> + if (n >= count || !repr || !repr[n]) {
>
> There are two input combinations to this function when repr is NULL.
>
> First is show_unknown=true and caps=0, which means the for_each_set_bit
> will not execute its body. (No bits set.)
>
> Second is show_unknown=false and caps=~0, which means count is zero so
> for_each_set_bit will again not run. (Bitfield size input param is zero.)
>
> So unless I am missing something I do not see the null pointer dereference.
>
> What could theoretically happen is that a third input combination
> appears, where caps is not zero in the show_unknown=true case, either
> via a fully un-handled engine->class (switch), or a new capability bit
> not added to the static array a bit above.
>
> That would assert during driver development here:
>
> if (GEM_WARN_ON(show_unknown))
>
> Granted that could be after the dereference in "if (n >= count ||
> !repr[n])", but would be caught in debug builds (CI) and therefore not
> be able to "ship" (get merge to the repo).
>
> Your second question is about empty buffer returned i.e. len=0 at the
> end of the function? (Which is when the buffer will not be null
> terminated - or you see another option?)
>
> That I think is safe too since it just results in a zero length read in
> sysfs.
>
> Regards,
>
> Tvrtko
>
>> if (GEM_WARN_ON(show_unknown))
>> len += sysfs_emit_at(buf, len, "[%x] ", n);
>> } else {
Thank you for such a full response.
I think you are right. I was under the impression that either currently
or in the future there might be an input combination, as you mentioned,
that may trigger the NULL dereference. If you feel it will be caught
beforehand, I am satisfied as well. Same goes for the empty buffer stuff.
I think dropping the patch is the best option then. Apologies for any
inconvenience.
Nikita
[PATCH] drm/amd/display: fix NULL checks for adev->dm.dc in amdgpu_dm_fini()
Since 'adev->dm.dc' in amdgpu_dm_fini() might turn out to be NULL
before the call to dc_enable_dmub_notifications(), check
beforehand to ensure there will not be a possible NULL-ptr-deref
there.
Also, since commit 1e88eb1b2c25 ("drm/amd/display: Drop
CONFIG_DRM_AMD_DC_HDCP") there are two separate checks for NULL in
'adev->dm.dc' before dc_deinit_callbacks() and dc_dmub_srv_destroy().
Clean up by combining them all under one 'if'.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 81927e2808be ("drm/amd/display: Support for DMUB AUX")
Signed-off-by: Nikita Zhandarovich
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c| 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index d292f290cd6e..46ac3e6f42bb 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1938,17 +1938,15 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
adev->dm.hdcp_workqueue = NULL;
}
- if (adev->dm.dc)
+ if (adev->dm.dc) {
dc_deinit_callbacks(adev->dm.dc);
-
- if (adev->dm.dc)
dc_dmub_srv_destroy(>dm.dc->ctx->dmub_srv);
-
- if (dc_enable_dmub_notifications(adev->dm.dc)) {
- kfree(adev->dm.dmub_notify);
- adev->dm.dmub_notify = NULL;
- destroy_workqueue(adev->dm.delayed_hpd_wq);
- adev->dm.delayed_hpd_wq = NULL;
+ if (dc_enable_dmub_notifications(adev->dm.dc)) {
+ kfree(adev->dm.dmub_notify);
+ adev->dm.dmub_notify = NULL;
+ destroy_workqueue(adev->dm.delayed_hpd_wq);
+ adev->dm.delayed_hpd_wq = NULL;
+ }
}
if (adev->dm.dmub_bo)
--
2.25.1
[PATCH] drm/radeon/ni: Fix wrong firmware size logging in ni_init_microcode()
Clean up a typo in pr_err() erroneously printing NI MC 'rdev->mc_fw->size'
during SMC firmware load. Log 'rdev->smc_fw->size' instead.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 6596afd48af4 ("drm/radeon/kms: add dpm support for btc (v3)")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/ni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 927e5f42e97d..3e48cbb522a1 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -813,7 +813,7 @@ int ni_init_microcode(struct radeon_device *rdev)
err = 0;
} else if (rdev->smc_fw->size != smc_req_size) {
pr_err("ni_mc: Bogus length %zu in firmware \"%s\"\n",
- rdev->mc_fw->size, fw_name);
+ rdev->smc_fw->size, fw_name);
err = -EINVAL;
}
}
--
2.25.1
[PATCH] drm/i915/gt: Prevent possible NULL dereference in __caps_show()
After falling through the switch statement to default case 'repr' is
initialized with NULL, which will lead to incorrect dereference of
'!repr[n]' in the following loop.
Fix it with the help of an additional check for NULL.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 4ec76dbeb62b ("drm/i915/gt: Expose engine properties via sysfs")
Signed-off-by: Nikita Zhandarovich
---
P.S. The NULL-deref problem might be dealt with this way but I am
not certain that the rest of the __caps_show() behaviour remains
correct if we end up in default case. For instance, as far as I
can tell, buf might turn out to be w/o '\0'. I could use some
direction if this has to be addressed as well.
drivers/gpu/drm/i915/gt/sysfs_engines.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c
b/drivers/gpu/drm/i915/gt/sysfs_engines.c
index 021f51d9b456..6b130b732867 100644
--- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
+++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
@@ -105,7 +105,7 @@ __caps_show(struct intel_engine_cs *engine,
len = 0;
for_each_set_bit(n, , show_unknown ? BITS_PER_LONG : count) {
- if (n >= count || !repr[n]) {
+ if (n >= count || !repr || !repr[n]) {
if (GEM_WARN_ON(show_unknown))
len += sysfs_emit_at(buf, len, "[%x] ", n);
} else {
--
2.25.1
[PATCH] drm/radeon/ni_dpm: remove redundant NULL check
'leakage_table' will always be successfully initialized as a pointer
to '>pm.dpm.dyn_state.cac_leakage_table'.
Remove unnecessary check if only to silence static checkers.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool Svace.
Fixes: 69e0b57a91ad ("drm/radeon/kms: add dpm support for cayman (v5)")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/ni_dpm.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index 3e1c1a392fb7..e08559c44a5c 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -3103,9 +3103,6 @@ static int ni_init_simplified_leakage_table(struct
radeon_device *rdev,
u32 smc_leakage, max_leakage = 0;
u32 scaling_factor;
- if (!leakage_table)
- return -EINVAL;
-
table_size = leakage_table->count;
if (eg_pi->vddc_voltage_table.count != table_size)
[PATCH] drm/radeon: remove dead code in ni_mc_load_microcode()
Inside the if block with (running == 0), the checks for 'running'
possibly being non-zero are redundant. Remove them altogether.
This change is similar to the one authored by Heinrich Schuchardt
in commit
ddbbd3be9679 ("drm/radeon: remove dead code, si_mc_load_microcode (v2)")
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool Svace.
Fixes: 0af62b016804 ("drm/radeon/kms: add ucode loader for NI")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/ni.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index 927e5f42e97d..8eac8c090433 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -624,7 +624,7 @@ static const u32 cayman_io_mc_regs[BTC_IO_MC_REGS_SIZE][2]
= {
int ni_mc_load_microcode(struct radeon_device *rdev)
{
const __be32 *fw_data;
- u32 mem_type, running, blackout = 0;
+ u32 mem_type, running;
u32 *io_mc_regs;
int i, ucode_size, regs_size;
@@ -659,11 +659,6 @@ int ni_mc_load_microcode(struct radeon_device *rdev)
running = RREG32(MC_SEQ_SUP_CNTL) & RUN_MASK;
if ((mem_type == MC_SEQ_MISC0_GDDR5_VALUE) && (running == 0)) {
- if (running) {
- blackout = RREG32(MC_SHARED_BLACKOUT_CNTL);
- WREG32(MC_SHARED_BLACKOUT_CNTL, 1);
- }
-
/* reset the engine and set to writable */
WREG32(MC_SEQ_SUP_CNTL, 0x0008);
WREG32(MC_SEQ_SUP_CNTL, 0x0010);
@@ -689,9 +684,6 @@ int ni_mc_load_microcode(struct radeon_device *rdev)
break;
udelay(1);
}
-
- if (running)
- WREG32(MC_SHARED_BLACKOUT_CNTL, blackout);
}
return 0;
[PATCH] drm/radeon/r600_cs: Fix possible int overflows in r600_cs_check_reg()
While improbable, there may be a chance of hitting integer
overflow when the result of radeon_get_ib_value() gets shifted
left.
Avoid it by casting one of the operands to larger data type (u64).
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 1729dd33d20b ("drm/radeon/kms: r600 CS parser fixes")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/r600_cs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index 638f861af80f..6cf54a747749 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -1275,7 +1275,7 @@ static int r600_cs_check_reg(struct radeon_cs_parser *p,
u32 reg, u32 idx)
return -EINVAL;
}
tmp = (reg - CB_COLOR0_BASE) / 4;
- track->cb_color_bo_offset[tmp] = radeon_get_ib_value(p, idx) <<
8;
+ track->cb_color_bo_offset[tmp] = (u64)radeon_get_ib_value(p,
idx) << 8;
ib[idx] += (u32)((reloc->gpu_offset >> 8) & 0x);
track->cb_color_base_last[tmp] = ib[idx];
track->cb_color_bo[tmp] = reloc->robj;
@@ -1302,7 +1302,7 @@ static int r600_cs_check_reg(struct radeon_cs_parser *p,
u32 reg, u32 idx)
"0x%04X\n", reg);
return -EINVAL;
}
- track->htile_offset = radeon_get_ib_value(p, idx) << 8;
+ track->htile_offset = (u64)radeon_get_ib_value(p, idx) << 8;
ib[idx] += (u32)((reloc->gpu_offset >> 8) & 0x);
track->htile_bo = reloc->robj;
track->db_dirty = true;
--
2.25.1
[PATCH] drm/radeon/r100: Fix integer overflow issues in r100_cs_track_check()
It may be possible, albeit unlikely, to encounter integer overflow
during the multiplication of several unsigned int variables, the
result being assigned to a variable 'size' of wider type.
Prevent this potential behaviour by converting one of the multiples
to unsigned long.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 0242f74d29df ("drm/radeon: clean up CS functions in r100.c")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/r100.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index affa9e0309b2..cfeca2694d5f 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -2321,7 +2321,7 @@ int r100_cs_track_check(struct radeon_device *rdev,
struct r100_cs_track *track)
switch (prim_walk) {
case 1:
for (i = 0; i < track->num_arrays; i++) {
- size = track->arrays[i].esize * track->max_indx * 4;
+ size = track->arrays[i].esize * track->max_indx * 4UL;
if (track->arrays[i].robj == NULL) {
DRM_ERROR("(PW %u) Vertex array %u no buffer "
"bound\n", prim_walk, i);
@@ -2340,7 +2340,7 @@ int r100_cs_track_check(struct radeon_device *rdev,
struct r100_cs_track *track)
break;
case 2:
for (i = 0; i < track->num_arrays; i++) {
- size = track->arrays[i].esize * (nverts - 1) * 4;
+ size = track->arrays[i].esize * (nverts - 1) * 4UL;
if (track->arrays[i].robj == NULL) {
DRM_ERROR("(PW %u) Vertex array %u no buffer "
"bound\n", prim_walk, i);
--
2.25.1
Re: [PATCH] video/hdmi: convert *_infoframe_init() functions to void
Hello,
On 8/10/23 01:13, Maxime Ripard wrote:
> Hi,
>
> On Tue, Aug 08, 2023 at 11:02:45AM -0700, Nikita Zhandarovich wrote:
>> Four hdmi_*_infoframe_init() functions that initialize different
>> types of hdmi infoframes only return the default 0 value, contrary to
>> their descriptions. Yet these functions are still unnecessarily checked
>> against possible errors in case of failure.
>>
>> Remove redundant error checks in calls to following functions:
>> - hdmi_spd_infoframe_init
>> - hdmi_audio_infoframe_init
>> - hdmi_vendor_infoframe_init
>> - hdmi_drm_infoframe_init
>> Also, convert these functions to 'void' and fix their descriptions.
>
> I'm not sure what value it actually adds. None of them return any
> errors, but very well might if we started to be a bit serious about it.
>
> Since the error handling is already there, then I'd rather leave it
> there.
There is definitely no particular urgency to this change.
Since these functions don't perform anything complex and aren't updated
regularly, my main goal was to remove unnecessary (at the moment) checks
and fix up their somewhat misleading descriptions. Cleaning up, in other
words. But I understand your point of view.
If you don't think this patch is warranted at this point, I totally
understand.
>
>> Fixes: 2c676f378edb ("[media] hdmi: added unpack and logging functions for
>> InfoFrames")
>
> I'm confused about that part. What does it fix exactly?
>
> Maxime
I added the 'Fixes:' tag mostly as a requirement for patch's
description. Once again, it doesn't "fix" anything broken as much as it
cleans up stuff.
Best regards,
Nikita
[PATCH] drm/radeon: check return value of radeon_ring_lock()
In the unlikely event of radeon_ring_lock() failing, its errno return
value should be processed. This patch checks said return value and
prints a debug message in case of an error.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 48c0c902e2e6 ("drm/radeon/kms: add support for CP setup on SI")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/si.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index 8d5e4b25609d..df1b2ebc37c2 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -3611,6 +3611,10 @@ static int si_cp_start(struct radeon_device *rdev)
for (i = RADEON_RING_TYPE_GFX_INDEX; i <= CAYMAN_RING_TYPE_CP2_INDEX;
++i) {
ring = >ring[i];
r = radeon_ring_lock(rdev, ring, 2);
+ if (r) {
+ DRM_ERROR("radeon: cp failed to lock ring (%d).\n", r);
+ return r;
+ }
/* clear the compute context state */
radeon_ring_write(ring, PACKET3_COMPUTE(PACKET3_CLEAR_STATE,
0));
--
2.25.1
[PATCH] video/hdmi: convert *_infoframe_init() functions to void
Four hdmi_*_infoframe_init() functions that initialize different
types of hdmi infoframes only return the default 0 value, contrary to
their descriptions. Yet these functions are still unnecessarily checked
against possible errors in case of failure.
Remove redundant error checks in calls to following functions:
- hdmi_spd_infoframe_init
- hdmi_audio_infoframe_init
- hdmi_vendor_infoframe_init
- hdmi_drm_infoframe_init
Also, convert these functions to 'void' and fix their descriptions.
Fixes: 2c676f378edb ("[media] hdmi: added unpack and logging functions for
InfoFrames")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/display/drm_hdmi_helper.c | 5 +---
drivers/gpu/drm/drm_edid.c| 5 +---
drivers/gpu/drm/i915/display/intel_hdmi.c | 7 ++---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 14 ++
drivers/gpu/drm/radeon/r600_hdmi.c| 6 +---
drivers/gpu/drm/sti/sti_hdmi.c| 6 +---
drivers/gpu/drm/tegra/hdmi.c | 7 +
drivers/gpu/drm/tegra/sor.c | 6 +---
drivers/gpu/drm/vc4/vc4_hdmi.c| 7 +
drivers/video/hdmi.c | 46 ++-
include/linux/hdmi.h | 10 +++
11 files changed, 25 insertions(+), 94 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
b/drivers/gpu/drm/display/drm_hdmi_helper.c
index faf5e9efa7d3..ce7038a3a183 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -27,7 +27,6 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
{
struct drm_connector *connector;
struct hdr_output_metadata *hdr_metadata;
- int err;
if (!frame || !conn_state)
return -EINVAL;
@@ -47,9 +46,7 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
connector->hdr_sink_metadata.hdmi_type1.eotf))
DRM_DEBUG_KMS("Unknown EOTF %d\n",
hdr_metadata->hdmi_metadata_type1.eotf);
- err = hdmi_drm_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_drm_infoframe_init(frame);
frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
frame->metadata_type = hdr_metadata->hdmi_metadata_type1.metadata_type;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index e0dbd9140726..d4933f215675 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -7235,7 +7235,6 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
*/
bool has_hdmi_infoframe = connector ?
connector->display_info.has_hdmi_infoframe : false;
- int err;
if (!frame || !mode)
return -EINVAL;
@@ -7243,9 +7242,7 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
if (!has_hdmi_infoframe)
return -EINVAL;
- err = hdmi_vendor_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_vendor_infoframe_init(frame);
/*
* Even if it's not absolutely necessary to send the infoframe
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 7ac5e6c5e00d..8b58127bca37 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -763,12 +763,9 @@ intel_hdmi_compute_spd_infoframe(struct intel_encoder
*encoder,
intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_SPD);
if (IS_DGFX(i915))
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
+ hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
else
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
-
- if (drm_WARN_ON(encoder->base.dev, ret))
- return false;
+ hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
frame->sdi = HDMI_SPD_SDI_PC;
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 0a8e0a13f516..75899e4a011f 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -995,12 +995,7 @@ static int mtk_hdmi_setup_spd_infoframe(struct mtk_hdmi
*hdmi,
u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_SPD_INFOFRAME_SIZE];
ssize_t err;
- err = hdmi_spd_infoframe_init(, vendor, product);
- if (err < 0) {
- dev_err(hdmi->dev, "Failed to initialize SPD infoframe: %zd\n",
- err);
- return err;
- }
+ hdmi_spd_infoframe_init(, vendor, product);
err = hdmi_spd_infoframe_pack(, buffer, sizeof(buffer));
[PATCH] drm/radeon: fix possible division-by-zero errors
Function rv740_get_decoded_reference_divider() may return 0 due to
unpredictable reference divider value calculated in
radeon_atom_get_clock_dividers(). This will lead to
division-by-zero error once that value is used as a divider
in calculating 'clk_s'.
While unlikely, this issue should nonetheless be prevented so add a
sanity check for such cases by testing 'decoded_ref' value against 0.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 66229b200598 ("drm/radeon/kms: add dpm support for rv7xx (v4)")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/cypress_dpm.c | 7 +--
drivers/gpu/drm/radeon/ni_dpm.c | 7 +--
drivers/gpu/drm/radeon/rv740_dpm.c | 7 +--
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/cypress_dpm.c
b/drivers/gpu/drm/radeon/cypress_dpm.c
index fdddbbaecbb7..3678b7e384e1 100644
--- a/drivers/gpu/drm/radeon/cypress_dpm.c
+++ b/drivers/gpu/drm/radeon/cypress_dpm.c
@@ -555,10 +555,13 @@ static int cypress_populate_mclk_value(struct
radeon_device *rdev,
if (radeon_atombios_get_asic_ss_info(rdev, ,
ASIC_INTERNAL_MEMORY_SS,
vco_freq)) {
+ u32 clk_s, clk_v;
u32 reference_clock = rdev->clock.mpll.reference_freq;
u32 decoded_ref =
rv740_get_decoded_reference_divider(dividers.ref_div);
- u32 clk_s = reference_clock * 5 / (decoded_ref *
ss.rate);
- u32 clk_v = ss.percentage *
+ if (!decoded_ref)
+ return -EINVAL;
+ clk_s = reference_clock * 5 / (decoded_ref * ss.rate);
+ clk_v = ss.percentage *
(0x4000 * dividers.whole_fb_div + 0x800 *
dividers.frac_fb_div) / (clk_s * 625);
mpll_ss1 &= ~CLKV_MASK;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index 672d2239293e..9ce3e5635efc 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2239,10 +2239,13 @@ static int ni_populate_mclk_value(struct radeon_device
*rdev,
if (radeon_atombios_get_asic_ss_info(rdev, ,
ASIC_INTERNAL_MEMORY_SS,
vco_freq)) {
+ u32 clk_s, clk_v;
u32 reference_clock = rdev->clock.mpll.reference_freq;
u32 decoded_ref =
rv740_get_decoded_reference_divider(dividers.ref_div);
- u32 clk_s = reference_clock * 5 / (decoded_ref *
ss.rate);
- u32 clk_v = ss.percentage *
+ if (!decoded_ref)
+ return -EINVAL;
+ clk_s = reference_clock * 5 / (decoded_ref * ss.rate);
+ clk_v = ss.percentage *
(0x4000 * dividers.whole_fb_div + 0x800 *
dividers.frac_fb_div) / (clk_s * 625);
mpll_ss1 &= ~CLKV_MASK;
diff --git a/drivers/gpu/drm/radeon/rv740_dpm.c
b/drivers/gpu/drm/radeon/rv740_dpm.c
index d57a3e1df8d6..ca76efa0f59d 100644
--- a/drivers/gpu/drm/radeon/rv740_dpm.c
+++ b/drivers/gpu/drm/radeon/rv740_dpm.c
@@ -247,10 +247,13 @@ int rv740_populate_mclk_value(struct radeon_device *rdev,
if (radeon_atombios_get_asic_ss_info(rdev, ,
ASIC_INTERNAL_MEMORY_SS,
vco_freq)) {
+ u32 clk_s, clk_v;
u32 reference_clock = rdev->clock.mpll.reference_freq;
u32 decoded_ref =
rv740_get_decoded_reference_divider(dividers.ref_div);
- u32 clk_s = reference_clock * 5 / (decoded_ref *
ss.rate);
- u32 clk_v = 0x4 * ss.percentage *
+ if (!decoded_ref)
+ return -EINVAL;
+ clk_s = reference_clock * 5 / (decoded_ref * ss.rate);
+ clk_v = 0x4 * ss.percentage *
(dividers.whole_fb_div + (dividers.frac_fb_div
/ 8)) / (clk_s * 1);
mpll_ss1 &= ~CLKV_MASK;
--
2.25.1
[PATCH] drm/i915/dp: prevent potential div-by-zero
drm_dp_dsc_sink_max_slice_count() may return 0 if something goes
wrong on the part of the DSC sink and its DPCD register. This null
value may be later used as a divisor in intel_dsc_compute_params(),
which will lead to an error.
In the unlikely event that this issue occurs, fix it by testing the
return value of drm_dp_dsc_sink_max_slice_count() against zero.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: a4a15c80 ("drm/i915/dp: Compute DSC pipe config in atomic check")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/i915/display/intel_dp.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 62cbab7402e9..c1825f8f885c 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1533,6 +1533,11 @@ int intel_dp_dsc_compute_config(struct intel_dp
*intel_dp,
pipe_config->dsc.slice_count =
drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
true);
+ if (!pipe_config->dsc.slice_count) {
+ drm_dbg_kms(_priv->drm, "Unsupported Slice Count
%d\n",
+ pipe_config->dsc.slice_count);
+ return -EINVAL;
+ }
} else {
u16 dsc_max_output_bpp = 0;
u8 dsc_dp_slice_count;
--
2.25.1
Re: [PATCH] drm/ttm: fix null-ptr-deref in radeon_ttm_tt_populate()
On 4/17/23 07:42, Christian König wrote:
>
>
> Am 17.04.23 um 16:34 schrieb Nikita Zhandarovich:
>> Currently, drm_prime_sg_to_page_addr_arrays() dereferences 'gtt->ttm'
>> without ensuring that 'gtt' (and therefore 'gtt->tmm') is not NULL.
>>
>> Fix this by testing 'gtt' for NULL value before dereferencing.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 40f5cf996991 ("drm/radeon: add PRIME support (v2)")
>> Signed-off-by: Nikita Zhandarovich
>> ---
>> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 1e8e287e113c..33d01c3bdee4 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -553,7 +553,7 @@ static int radeon_ttm_tt_populate(struct
>> ttm_device *bdev,
>> return 0;
>> }
>> - if (slave && ttm->sg) {
>> + if (gtt && slave && ttm->sg) {
>
> The gtt variable is derived from the ttm variable and so never NULL
> here. The only case when this can be NULL is for AGP and IIRC we don't
> support DMA-buf in this case.
>
>> drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
>
> Just use ttm->dma_addresses instead of gtt->ttm.dma_address here to make
> your automated checker happy.
>
> Regards,
> Christian.
>
>> ttm->num_pages);
>> return 0;
>
Thank you for your reply, you are absolutely right.
Apologies for wasting your time.
Nikita
[PATCH] drm/ttm: fix null-ptr-deref in radeon_ttm_tt_populate()
Currently, drm_prime_sg_to_page_addr_arrays() dereferences 'gtt->ttm'
without ensuring that 'gtt' (and therefore 'gtt->tmm') is not NULL.
Fix this by testing 'gtt' for NULL value before dereferencing.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 40f5cf996991 ("drm/radeon: add PRIME support (v2)")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 1e8e287e113c..33d01c3bdee4 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -553,7 +553,7 @@ static int radeon_ttm_tt_populate(struct ttm_device *bdev,
return 0;
}
- if (slave && ttm->sg) {
+ if (gtt && slave && ttm->sg) {
drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
ttm->num_pages);
return 0;
--
2.25.1
[PATCH v2] radeon: avoid double free in ci_dpm_init()
Several calls to ci_dpm_fini() will attempt to free resources that
either have been freed before or haven't been allocated yet. This
may lead to undefined or dangerous behaviour.
For instance, if r600_parse_extended_power_table() fails, it might
call r600_free_extended_power_table() as will ci_dpm_fini() later
during error handling.
Fix this by only freeing pointers to objects previously allocated.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
Cc: sta...@vger.kernel.org
Co-developed-by: Natalia Petrova
Signed-off-by: Nikita Zhandarovich
---
v2: free only resouces allocated prior, do not remove ci_dpm_fini()
or other deallocating calls altogether; fix commit message.
v1: https://lore.kernel.org/all/20230403182808.8699-1-n.zhandarov...@fintech.ru/
drivers/gpu/drm/radeon/ci_dpm.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
index 8ef25ab305ae..b8f4dac68d85 100644
--- a/drivers/gpu/drm/radeon/ci_dpm.c
+++ b/drivers/gpu/drm/radeon/ci_dpm.c
@@ -5517,6 +5517,7 @@ static int ci_parse_power_table(struct radeon_device
*rdev)
u8 frev, crev;
u8 *power_state_offset;
struct ci_ps *ps;
+ int ret;
if (!atom_parse_data_header(mode_info->atom_context, index, NULL,
, , _offset))
@@ -5546,11 +5547,15 @@ static int ci_parse_power_table(struct radeon_device
*rdev)
non_clock_array_index = power_state->v2.nonClockInfoIndex;
non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
_clock_info_array->nonClockInfo[non_clock_array_index];
- if (!rdev->pm.power_state[i].clock_info)
- return -EINVAL;
+ if (!rdev->pm.power_state[i].clock_info) {
+ ret = -EINVAL;
+ goto err_free_ps;
+ }
ps = kzalloc(sizeof(struct ci_ps), GFP_KERNEL);
- if (ps == NULL)
- return -ENOMEM;
+ if (ps == NULL) {
+ ret = -ENOMEM;
+ goto err_free_ps;
+ }
rdev->pm.dpm.ps[i].ps_priv = ps;
ci_parse_pplib_non_clock_info(rdev, >pm.dpm.ps[i],
non_clock_info,
@@ -5590,6 +5595,12 @@ static int ci_parse_power_table(struct radeon_device
*rdev)
}
return 0;
+
+err_free_ps:
+ for (i = 0; i < rdev->pm.dpm.num_ps; i++)
+ kfree(rdev->pm.dpm.ps[i].ps_priv);
+ kfree(rdev->pm.dpm.ps);
+ return ret;
}
static int ci_get_vbios_boot_values(struct radeon_device *rdev,
@@ -5678,25 +5689,26 @@ int ci_dpm_init(struct radeon_device *rdev)
ret = ci_get_vbios_boot_values(rdev, >vbios_boot_state);
if (ret) {
- ci_dpm_fini(rdev);
+ kfree(rdev->pm.dpm.priv);
return ret;
}
ret = r600_get_platform_caps(rdev);
if (ret) {
- ci_dpm_fini(rdev);
+ kfree(rdev->pm.dpm.priv);
return ret;
}
ret = r600_parse_extended_power_table(rdev);
if (ret) {
- ci_dpm_fini(rdev);
+ kfree(rdev->pm.dpm.priv);
return ret;
}
ret = ci_parse_power_table(rdev);
if (ret) {
- ci_dpm_fini(rdev);
+ kfree(rdev->pm.dpm.priv);
+ r600_free_extended_power_table(rdev);
return ret;
}
[PATCH v2] video/hdmi: minor fixes for *_infoframe_init functions
Multiple hdmi_*_infoframe_init() functions that initialize different
types of hdmi infoframes only return default 0 value (contrary to their
descriptions). Yet these functions are still checked against possible
errors in case of failure.
This patch removes redundant checks for errors in calls to following
functions:
- hdmi_spd_infoframe_init
- hdmi_audio_infoframe_init
- hdmi_vendor_infoframe_init
- hdmi_drm_infoframe_init
Also, change their return types to void and fix descriptions.
Fixes: 2c676f378edb ("[media] hdmi: added unpack and logging functions for
InfoFrames")
Signed-off-by: Nikita Zhandarovich
---
v2: Fix build warning by removing unnecessary call to drm_WARN_ON()
with uninitialized value.
Reported-by: kernel test robot
Link:
https://lore.kernel.org/oe-kbuild-all/202304131234.ht3mzkju-...@intel.com/
drivers/gpu/drm/display/drm_hdmi_helper.c | 5 +---
drivers/gpu/drm/drm_edid.c| 5 +---
drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +--
drivers/gpu/drm/mediatek/mtk_hdmi.c | 14 ++
drivers/gpu/drm/radeon/r600_hdmi.c
drivers/gpu/drm/display/drm_hdmi_helper.c | 5 +---
drivers/gpu/drm/drm_edid.c| 5 +---
drivers/gpu/drm/i915/display/intel_hdmi.c | 7 ++---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 14 ++
drivers/gpu/drm/radeon/r600_hdmi.c| 6 +
drivers/gpu/drm/sti/sti_hdmi.c| 6 +
drivers/gpu/drm/tegra/hdmi.c | 7 +
drivers/gpu/drm/tegra/sor.c | 6 +
drivers/gpu/drm/vc4/vc4_hdmi.c| 7 +
drivers/video/hdmi.c | 44 ++-
include/linux/hdmi.h | 8 +++---
11 files changed, 23 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
b/drivers/gpu/drm/display/drm_hdmi_helper.c
index faf5e9efa7d3..ce7038a3a183 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -27,7 +27,6 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
{
struct drm_connector *connector;
struct hdr_output_metadata *hdr_metadata;
- int err;
if (!frame || !conn_state)
return -EINVAL;
@@ -47,9 +46,7 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
connector->hdr_sink_metadata.hdmi_type1.eotf))
DRM_DEBUG_KMS("Unknown EOTF %d\n",
hdr_metadata->hdmi_metadata_type1.eotf);
- err = hdmi_drm_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_drm_infoframe_init(frame);
frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
frame->metadata_type = hdr_metadata->hdmi_metadata_type1.metadata_type;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 261a62e15934..c268148502d6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -7159,7 +7159,6 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
*/
bool has_hdmi_infoframe = connector ?
connector->display_info.has_hdmi_infoframe : false;
- int err;
if (!frame || !mode)
return -EINVAL;
@@ -7167,9 +7166,7 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
if (!has_hdmi_infoframe)
return -EINVAL;
- err = hdmi_vendor_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_vendor_infoframe_init(frame);
/*
* Even if it's not absolutely necessary to send the infoframe
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index c0ce6d3dc505..59e2f53015c0 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -778,12 +778,9 @@ intel_hdmi_compute_spd_infoframe(struct intel_encoder
*encoder,
intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_SPD);
if (IS_DGFX(i915))
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
+ hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
else
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
-
- if (drm_WARN_ON(encoder->base.dev, ret))
- return false;
+ hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
frame->sdi = HDMI_SPD_SDI_PC;
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 0a8e0a13f516..75899e4a011f 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -995,12 +995,7 @@ static int mtk_hdmi_setup_spd_infoframe(struct mtk_hdmi
*hdmi,
[PATCH] video/hdmi: minor fixes for *_infoframe_init functions
Multiple hdmi_*_infoframe_init() functions that initialize different
types of hdmi infoframes only return default 0 value (contrary to their
descriptions). Yet these functions are still checked against possible
errors in case of failure.
This patch removes redundant checks for errors in calls to following
functions:
- hdmi_spd_infoframe_init
- hdmi_audio_infoframe_init
- hdmi_vendor_infoframe_init
- hdmi_drm_infoframe_init
Also, change their return types to void and fix descriptions.
Fixes: 2c676f378edb ("[media] hdmi: added unpack and logging functions for
InfoFrames")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/display/drm_hdmi_helper.c | 5 +---
drivers/gpu/drm/drm_edid.c| 5 +---
drivers/gpu/drm/i915/display/intel_hdmi.c | 4 +--
drivers/gpu/drm/mediatek/mtk_hdmi.c | 14 ++
drivers/gpu/drm/radeon/r600_hdmi.c| 6 +
drivers/gpu/drm/sti/sti_hdmi.c| 6 +
drivers/gpu/drm/tegra/hdmi.c | 7 +
drivers/gpu/drm/tegra/sor.c | 6 +
drivers/gpu/drm/vc4/vc4_hdmi.c| 7 +
drivers/video/hdmi.c | 44 ++-
include/linux/hdmi.h | 8 +++---
11 files changed, 23 insertions(+), 89 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
b/drivers/gpu/drm/display/drm_hdmi_helper.c
index faf5e9efa7d3..ce7038a3a183 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -27,7 +27,6 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
{
struct drm_connector *connector;
struct hdr_output_metadata *hdr_metadata;
- int err;
if (!frame || !conn_state)
return -EINVAL;
@@ -47,9 +46,7 @@ int drm_hdmi_infoframe_set_hdr_metadata(struct
hdmi_drm_infoframe *frame,
connector->hdr_sink_metadata.hdmi_type1.eotf))
DRM_DEBUG_KMS("Unknown EOTF %d\n",
hdr_metadata->hdmi_metadata_type1.eotf);
- err = hdmi_drm_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_drm_infoframe_init(frame);
frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
frame->metadata_type = hdr_metadata->hdmi_metadata_type1.metadata_type;
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 261a62e15934..c268148502d6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -7159,7 +7159,6 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
*/
bool has_hdmi_infoframe = connector ?
connector->display_info.has_hdmi_infoframe : false;
- int err;
if (!frame || !mode)
return -EINVAL;
@@ -7167,9 +7166,7 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct
hdmi_vendor_infoframe *frame,
if (!has_hdmi_infoframe)
return -EINVAL;
- err = hdmi_vendor_infoframe_init(frame);
- if (err < 0)
- return err;
+ hdmi_vendor_infoframe_init(frame);
/*
* Even if it's not absolutely necessary to send the infoframe
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index c0ce6d3dc505..2190dcf68f7f 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -778,9 +778,9 @@ intel_hdmi_compute_spd_infoframe(struct intel_encoder
*encoder,
intel_hdmi_infoframe_enable(HDMI_INFOFRAME_TYPE_SPD);
if (IS_DGFX(i915))
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
+ hdmi_spd_infoframe_init(frame, "Intel", "Discrete gfx");
else
- ret = hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
+ hdmi_spd_infoframe_init(frame, "Intel", "Integrated gfx");
if (drm_WARN_ON(encoder->base.dev, ret))
return false;
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 0a8e0a13f516..75899e4a011f 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -995,12 +995,7 @@ static int mtk_hdmi_setup_spd_infoframe(struct mtk_hdmi
*hdmi,
u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_SPD_INFOFRAME_SIZE];
ssize_t err;
- err = hdmi_spd_infoframe_init(, vendor, product);
- if (err < 0) {
- dev_err(hdmi->dev, "Failed to initialize SPD infoframe: %zd\n",
- err);
- return err;
- }
+ hdmi_spd_infoframe_init(, vendor, product);
err = hdmi_spd_infoframe_pack(, buffer, sizeof(buffer));
if (err < 0) {
@@ -1018,12 +1013,7 @@ static int mtk_hdmi_setup_
Re: [PATCH] radeon: avoid double free in ci_dpm_init()
On 4/11/23 14:11, Deucher, Alexander wrote:
> [Public]
>
>> -Original Message-----
>> From: Nikita Zhandarovich
>> Sent: Monday, April 3, 2023 2:28 PM
>> To: Deucher, Alexander
>> Cc: Nikita Zhandarovich ; Koenig, Christian
>> ; Pan, Xinhui ; David
>> Airlie ; Daniel Vetter ; amd-
>> g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
>> ker...@vger.kernel.org; lvc-proj...@linuxtesting.org
>> Subject: [PATCH] radeon: avoid double free in ci_dpm_init()
>>
>> There are several calls to ci_dpm_fini() in ci_dpm_init() when there occur
>> errors in functions like r600_parse_extended_power_table().
>> This is harmful as it can lead to double free situations: for instance,
>> r600_parse_extended_power_table() will call for
>> r600_free_extended_power_table() as will ci_dpm_fini(), both of which will
>> try to free resources.
>> Other drivers do not call *_dpm_fini functions from their respective
>> *_dpm_init calls - neither should cpm_dpm_init().
>>
>> Fix this by removing extra calls to ci_dpm_fini().
>
> You can't just drop the calls to fini(). You'll need to properly unwind to
> avoid leaking memory.
>
> Alex
>>>
>> Found by Linux Verification Center (linuxtesting.org) with static analysis
>> tool
>> SVACE.
>>
>> Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
>> Cc: sta...@vger.kernel.org
>> Co-developed-by: Natalia Petrova
>> Signed-off-by: Nikita Zhandarovich
>>
>> ---
>> drivers/gpu/drm/radeon/ci_dpm.c | 20 +---
>> 1 file changed, 5 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/ci_dpm.c
>> b/drivers/gpu/drm/radeon/ci_dpm.c index 8ef25ab305ae..7b77d4c93f1d
>> 100644
>> --- a/drivers/gpu/drm/radeon/ci_dpm.c
>> +++ b/drivers/gpu/drm/radeon/ci_dpm.c
>> @@ -5677,28 +5677,20 @@ int ci_dpm_init(struct radeon_device *rdev)
>> pi->pcie_lane_powersaving.min = 16;
>>
>> ret = ci_get_vbios_boot_values(rdev, >vbios_boot_state);
>> -if (ret) {
>> -ci_dpm_fini(rdev);
>> +if (ret)
>> return ret;
>> -}
>>
>> ret = r600_get_platform_caps(rdev);
>> -if (ret) {
>> -ci_dpm_fini(rdev);
>> +if (ret)
>> return ret;
>> -}
>>
>> ret = r600_parse_extended_power_table(rdev);
>> -if (ret) {
>> -ci_dpm_fini(rdev);
>> +if (ret)
>> return ret;
>> -}
>>
>> ret = ci_parse_power_table(rdev);
>> -if (ret) {
>> -ci_dpm_fini(rdev);
>> +if (ret)
>> return ret;
>> -}
>>
>> pi->dll_default_on = false;
>> pi->sram_end = SMC_RAM_END;
>> @@ -5749,10 +5741,8 @@ int ci_dpm_init(struct radeon_device *rdev)
>> kcalloc(4,
>> sizeof(struct
>> radeon_clock_voltage_dependency_entry),
>> GFP_KERNEL);
>> -if (!rdev-
>>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) {
>> -ci_dpm_fini(rdev);
>> +if (!rdev-
>>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries)
>> return -ENOMEM;
>> -}
>> rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.count = 4;
>> rdev-
>>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].clk = 0;
>> rdev-
>>> pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].v = 0;
I think you are correct when it comes to ensuring we deal with memory
issues in ci_dpm_init().
However, I could use some direction on how to deal with the problem of
freeing only previously allocated resources. For instance, once
ci_parse_power_table() fails, it is not clear what we should and should
not free.
I wanna point out that in this case I would like to fix both double and
uninitialized free issues as it can also lead to undefined behavior.
Thanks for your patience,
Nikita
[PATCH] radeon: avoid double free in ci_dpm_init()
There are several calls to ci_dpm_fini() in ci_dpm_init() when there
occur errors in functions like r600_parse_extended_power_table().
This is harmful as it can lead to double free situations: for
instance, r600_parse_extended_power_table() will call for
r600_free_extended_power_table() as will ci_dpm_fini(), both
of which will try to free resources.
Other drivers do not call *_dpm_fini functions from their
respective *_dpm_init calls - neither should cpm_dpm_init().
Fix this by removing extra calls to ci_dpm_fini().
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
Cc: sta...@vger.kernel.org
Co-developed-by: Natalia Petrova
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/ci_dpm.c | 20 +---
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
index 8ef25ab305ae..7b77d4c93f1d 100644
--- a/drivers/gpu/drm/radeon/ci_dpm.c
+++ b/drivers/gpu/drm/radeon/ci_dpm.c
@@ -5677,28 +5677,20 @@ int ci_dpm_init(struct radeon_device *rdev)
pi->pcie_lane_powersaving.min = 16;
ret = ci_get_vbios_boot_values(rdev, >vbios_boot_state);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = r600_get_platform_caps(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = r600_parse_extended_power_table(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = ci_parse_power_table(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
pi->dll_default_on = false;
pi->sram_end = SMC_RAM_END;
@@ -5749,10 +5741,8 @@ int ci_dpm_init(struct radeon_device *rdev)
kcalloc(4,
sizeof(struct radeon_clock_voltage_dependency_entry),
GFP_KERNEL);
- if (!rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) {
- ci_dpm_fini(rdev);
+ if (!rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries)
return -ENOMEM;
- }
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.count = 4;
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].clk = 0;
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].v = 0;
[PATCH] drm/radeon: Fix potential null-ptr-deref
radeon_get_connector_for_encoder() assigns radeon_encoder->enc_priv to
mst_enc which is dereferenced later without being checked for NULL
beforehand. It is possible for radeon_encoder->enc_priv and therefore
mst_enc, to be NULL due to potential lack of memory.
This patch adds a sanity NULL-check to prevent the issue.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 9843ead08f18 ("drm/radeon: add DisplayPort MST support (v2)")
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/radeon_encoders.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_encoders.c
b/drivers/gpu/drm/radeon/radeon_encoders.c
index 46549d5179ee..3f68f0af443a 100644
--- a/drivers/gpu/drm/radeon/radeon_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_encoders.c
@@ -251,6 +251,9 @@ radeon_get_connector_for_encoder(struct drm_encoder
*encoder)
continue;
mst_enc = radeon_encoder->enc_priv;
+ if (!mst_enc)
+ return NULL;
+
if (mst_enc->connector == radeon_connector->mst_port)
return connector;
} else if (radeon_encoder->active_device &
radeon_connector->devices)
--
2.25.1
[PATCH] drm/radeon: Fix potential null-ptr-deref
Due to my rookie mistake this patch isn't necessary anymore in upstream version. The issue was already resolved in a different manner. Apologies for inconvenience.
