[lvc-project] [PATCH 0/3] drm/rockchip: avoid overflow of clock rate

[Fedor Pchelkin]

On Wed, 05. Nov 19:07, Karina Yankevich wrote:
> This patchset fixes conversion of the clock frequency from kHz to Hz in
> Rockchip DRM driver to avoid potential integer overflow on 64-bit arches
> (it is not possible to avoid on 32-bit arches due to the clk API using
> 'unsigned long' for the clock frequency).

Использование unsigned long в clk API обусловлено необходимостью поддержки
32-битных систем, где с 64-битной арифметикой и, в частности, операциями
деления, часто используемыми при вычислении частот, компилятором
генерируется неоптимальный (плохой) код.  Легаси, с которым фреймворку
приходится жить..

Данный драйвер от rockchip совместим в том числе с 32-битными SoC.
Большое количество паттернов (mode->clock * 1000) в ядре также намекает,
что ситуация переполнения то ли никому не интересна, то ли диапазон
mode->clock неявно определяется с помощью некоторых механизмов.

Стандартные поддерживаемые на текущий момент drm-подсистемой моды
перечислены в файле drivers/gpu/drm/drm_edid.c.  Макс. частота, которая
может быть, равна 594000 КГц для случаев "10240x4320 at 120Hz 64:27" и
"10240x4320 at 100Hz 64:27".  Если в rockchip-vop драйвер попадёт такой мод,
действительно может произойти переполнение в 32-битах при умножении на
1000.  Следующая в порядке уменьшения частота мода уже может быть равна
только 2970000 КГц, что вроде бы влезает в 32 бита, но будут проблемы с
расширением из int в unsigned long из-за старшего единичного бита, что
тоже плохо.

drm-драйверы определяют struct drm_crtc_helper_funcs, в которых есть
колбэк для проверки модов, выставляемых для crtc.

static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
	.mode_valid = vop_crtc_mode_valid,
	...
}

Внутри vop_crtc_mode_valid() есть проверка

	if (vop->data->max_output.width && mode->hdisplay > vop->data->max_output.width)
		return MODE_BAD_HVALUE;

добавленная коммитом 8e140cb60270 ("drm/rockchip: vop: limit maximum
resolution to hardware capabilities").  И если мод берётся из
предопределённых в drivers/gpu/drm/drm_edid.c, то моды с высокими
значениями clock эту проверку тоже не пройдут (у них большой hdisplay).


Однако проблема, насколько понимаю, может возникнуть из ioctl-обработчика
drm_mode_setcrtc(), который даёт возможность пользователю с правами
DRM_MASTER выставлять свой кастомный мод.  Базовые проверки для нового
передаваемого им clock проводятся в функциях drm_mode_convert_umode() и
drm_mode_validate_driver(), но они вроде как не покрывают то, что нужно..

Вообще говоря это выглядит более глобальной что ли проблемой, которую на
самом деле  не совсем понятно как целесообразно править.  Если через
указанный ioctl пользователь вправе выставлять почти любую конфигурацию
crtc / mode, что ему заблагорассудится, то при желании он может сделать
почти всё что угодно, что закончится чёрным экраном.  От переполнения
в данном случае я тоже пока вижу только влияние на это.

Есть может быть доп. мысли по этому поводу?  Описание патчей сейчас не
поясняет контекста или другой мотивации приведения типов переменных кроме
синтаксической.



Насчёт dw-hdmi немного по-другому - там есть проверка в
dw_hdmi_rockchip_mode_valid(), которая не должна дать выставить высокий
clock:

dw_hdmi_rockchip_mode_valid(struct dw_hdmi *dw_hdmi, void *data,
			    const struct drm_display_info *info,
			    const struct drm_display_mode *mode)
{
	struct rockchip_hdmi *hdmi = data;
	int pclk = mode->clock * 1000;

	if (hdmi->chip_data->max_tmds_clock &&
	    mode->clock > hdmi->chip_data->max_tmds_clock)
		return MODE_CLOCK_HIGH;
	...
}

> 
> Karina Yankevich (3):
>   drm/rockchip: vop: avoid overflow of clock rate in
>     vop_crtc_mode_fixup()
>   drm/rockchip: vop: avoid overflow of clock rate in
>     vop_crtc_atomic_enable()
>   drm/rockchip: dw_hdmi: avoid overflow of clock rate in
>     dw_hdmi_rockchip_encoder_mode_set()
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.34.1