Re: [PATCH] drm/radeon: Add NULL pointer check for enc_priv in atombios_set_encoder_crtc_source()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for NULL pointer dereference. The patch adds a local check for `enc_priv` (`dig`) in `atombios_set_encoder_crtc_source()`, but leaves the root cause unaddressed. In `radeon_add_atom_encoder()`, if the memory allocation for `enc_priv` fails (e.g., via `radeon_atombios_get_lvds_info()`, `radeon_atombios_set_dig_info()`, or `radeon_atombios_set_dac_info()`), the error is ignored and the encoder is registered with a NULL `enc_priv`. This broken encoder is then passed to numerous other functions (like `atombios_dig_encoder_setup2`, `atombios_dig_transmitter_setup2`, and `atombios_dac_setup`) which unconditionally dereference `enc_priv` (via `dig` or `dac_info`), leading to guaranteed kernel panics. The proper fix is to handle the allocation failure at the source and abort encoder registration.
- [High] The driver automatically defaults the display scaling mode to RMX_FULL for LCD devices, which violates the DRM subsystem guideline on scaling modes. This stretches the image and distorts its aspect ratio (e.g., circles become ovals) instead of preserving it.
--

commit 21561a0f42099aab9f6cd586dc86a1e791580aef
Author: Alexey Velichayshiy <a.velichayshiy@ispras.ru>

drm/radeon: Add NULL pointer check for enc_priv in atombios_set_encoder_crtc_source()

This commit adds a NULL pointer check for enc_priv in
atombios_set_encoder_crtc_source() to prevent a NULL pointer dereference.
The issue was reported to be found with SVACE.

> diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
> index 3d9f47bc807af..6efb6bd2d344c 100644
> --- a/drivers/gpu/drm/radeon/atombios_encoders.c
> +++ b/drivers/gpu/drm/radeon/atombios_encoders.c

[ ... ]

> @@ -1931,6 +1931,8 @@ atombios_set_encoder_crtc_source(struct drm_encoder *encoder)
>  			case ENCODER_OBJECT_ID_INTERNAL_UNIPHY3:
>  			case ENCODER_OBJECT_ID_INTERNAL_KLDSCP_LVTMA:
>  				dig = radeon_encoder->enc_priv;
> +				if (!dig)
> +					return;

[Severity: High]
This isn't a bug introduced by this patch, but does this local check leave
the root cause unaddressed?

In drivers/gpu/drm/radeon/atombios_encoders.c:radeon_add_atom_encoder(), if
the memory allocation for enc_priv fails (e.g. via
radeon_atombios_set_dig_info()), the error is ignored and the encoder is
registered with a NULL enc_priv:

radeon_add_atom_encoder() {
    ...
    radeon_encoder->enc_priv = radeon_atombios_set_dig_info(radeon_encoder);
    ...
}

This broken encoder is then passed to numerous other functions which
unconditionally dereference enc_priv, such as:

atombios_dig_encoder_setup2() {
    ...
    if (dig->dig_encoder == -1)
    ...
}

atombios_dig_transmitter_setup2() {
    ...
    int dig_encoder = dig->dig_encoder;
    ...
}

atombios_dac_setup() {
    ...
    switch (dac_info->tv_std) {
    ...
}

Since other DRM callbacks will unconditionally dereference it, could we handle
the allocation failure at the source and abort encoder registration instead?

[Severity: High]
This is a pre-existing issue, but does this driver incorrectly default to
distorting the aspect ratio for LCD devices?

Also in drivers/gpu/drm/radeon/atombios_encoders.c:radeon_add_atom_encoder():

radeon_add_atom_encoder() {
    ...
    radeon_encoder->rmx_type = RMX_FULL;
    ...
}

Setting the default scaling mode to RMX_FULL stretches the image and
distorts its aspect ratio (e.g., circles become ovals). Should this
default to RMX_ASPECT instead to preserve the aspect ratio?

>  				switch (dig->dig_encoder) {
>  				case 0:
>  					args.v2.ucEncoderID = ASIC_INT_DIG1_ENCODER_ID;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620183150.1248791-1-a.velichayshiy@ispras.ru?part=1