On 2024-06-05 10:53, Srinivasan Shanmugam wrote:
> This commit adds a null check for the 'afb' variable in the
> amdgpu_dm_update_cursor function. Previously, 'afb' was assumed to be
> null at line 8388, but was used later in the code without a null check.
> This could potentially lead to a null pointer dereference.
>
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:8433 amdgpu_dm_update_cursor()
> error: we previously assumed 'afb' could be null (see line 8388)
>
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
> 8379 static void amdgpu_dm_update_cursor(struct drm_plane *plane,
> 8380 struct drm_plane_state *old_plane_state,
> 8381 struct dc_stream_update *update)
> 8382 {
> 8383 struct amdgpu_device *adev = drm_to_adev(plane->dev);
> 8384 struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(plane->state->fb);
> 8385 struct drm_crtc *crtc = afb ? plane->state->crtc : old_plane_state->crtc;
> ^^^^^
>
> 8386 struct dm_crtc_state *crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL;
> 8387 struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> 8388 uint64_t address = afb ? afb->address : 0;
> ^^^^^ Checks for NULL
>
> 8389 struct dc_cursor_position position = {0};
> 8390 struct dc_cursor_attributes attributes;
> 8391 int ret;
> 8392
> 8393 if (!plane->state->fb && !old_plane_state->fb)
> 8394 return;
> 8395
> 8396 drm_dbg_atomic(plane->dev, "crtc_id=%d with size %d to %d\n",
> 8397 amdgpu_crtc->crtc_id, plane->state->crtc_w,
> 8398 plane->state->crtc_h);
> 8399
> 8400 ret = amdgpu_dm_plane_get_cursor_position(plane, crtc, &position);
> 8401 if (ret)
> 8402 return;
> 8403
> 8404 if (!position.enable) {
> 8405 /* turn off cursor */
> 8406 if (crtc_state && crtc_state->stream) {
> 8407 dc_stream_set_cursor_position(crtc_state->stream,
> 8408 &position);
> 8409 update->cursor_position = &crtc_state->stream->cursor_position;
> 8410 }
> 8411 return;
> 8412 }
> 8413
> 8414 amdgpu_crtc->cursor_width = plane->state->crtc_w;
> 8415 amdgpu_crtc->cursor_height = plane->state->crtc_h;
> 8416
> 8417 memset(&attributes, 0, sizeof(attributes));
> 8418 attributes.address.high_part = upper_32_bits(address);
> 8419 attributes.address.low_part = lower_32_bits(address);
> 8420 attributes.width = plane->state->crtc_w;
> 8421 attributes.height = plane->state->crtc_h;
> 8422 attributes.color_format = CURSOR_MODE_COLOR_PRE_MULTIPLIED_ALPHA;
> 8423 attributes.rotation_angle = 0;
> 8424 attributes.attribute_flags.value = 0;
> 8425
> 8426 /* Enable cursor degamma ROM on DCN3+ for implicit sRGB degamma in DRM
> 8427 * legacy gamma setup.
> 8428 */
> 8429 if (crtc_state->cm_is_degamma_srgb &&
> 8430 adev->dm.dc->caps.color.dpp.gamma_corr)
> 8431 attributes.attribute_flags.bits.ENABLE_CURSOR_DEGAMMA = 1;
> 8432
> --> 8433 attributes.pitch = afb->base.pitches[0] / afb->base.format->cpp[0];
> ^^^^^ ^^^^^
> Unchecked dereferences
>
> 8434
> 8435 if (crtc_state->stream) {
> 8436 if (!dc_stream_set_cursor_attributes(crtc_state->stream,
> 8437 &attributes))
> 8438 DRM_ERROR("DC failed to set cursor attributes\n");
> 8439
> 8440 update->cursor_attributes = &crtc_state->stream->cursor_attributes;
> 8441
> 8442 if (!dc_stream_set_cursor_position(crtc_state->stream,
> 8443 &position))
> 8444 DRM_ERROR("DC failed to set cursor position\n");
> 8445
> 8446 update->cursor_position = &crtc_state->stream->cursor_position;
> 8447 }
> 8448 }
>
> Fixes: 66eba12a5482 ("drm/amd/display: Do cursor programming with rest of pipe")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: Tom Chung <chiahsuan.chung@xxxxxxx>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
> Cc: Roman Li <roman.li@xxxxxxx>
> Cc: Hersen Wu <hersenxs.wu@xxxxxxx>
> Cc: Alex Hung <alex.hung@xxxxxxx>
> Cc: Aurabindo Pillai <aurabindo.pillai@xxxxxxx>
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
This code comes from amdgpu_dm_plane_handle_cursor_update. Would be
good to fix the same problem in that function as well.
Reviewed-by: Harry Wentland <harry.wentland@xxxxxxx>
Harry
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 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 6196de6cebbf..6d468badb669 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8637,14 +8637,22 @@ static void amdgpu_dm_update_cursor(struct drm_plane *plane,
> {
> struct amdgpu_device *adev = drm_to_adev(plane->dev);
> struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(plane->state->fb);
> - struct drm_crtc *crtc = afb ? plane->state->crtc : old_plane_state->crtc;
> - struct dm_crtc_state *crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL;
> - struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> - uint64_t address = afb ? afb->address : 0;
> + struct drm_crtc *crtc;
> + struct dm_crtc_state *crtc_state;
> + struct amdgpu_crtc *amdgpu_crtc;
> + u64 address;
> struct dc_cursor_position position = {0};
> struct dc_cursor_attributes attributes;
> int ret;
>
> + if (!afb)
> + return;
> +
> + crtc = plane->state->crtc ? plane->state->crtc : old_plane_state->crtc;
> + crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL;
> + amdgpu_crtc = to_amdgpu_crtc(crtc);
> + address = afb->address;
> +
> if (!plane->state->fb && !old_plane_state->fb)
> return;
>
