linux-kernel - Re: [PATCH] drm/mediatek: Init `ddp_comp` with devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 1 Apr 2024 03:33:57 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>, "dianders@...omium.org"
	<dianders@...omium.org>, "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>,
	"daniel@...ll.ch" <daniel@...ll.ch>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, Nathan Lu (呂東霖)
	<Nathan.Lu@...iatek.com>, "airlied@...il.com" <airlied@...il.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH] drm/mediatek: Init `ddp_comp` with devm_kcalloc()

Hi, Douglas:

On Thu, 2024-03-28 at 09:22 -0700, Douglas Anderson wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  In the case where `conn_routes` is true we allocate an extra slot in
> the `ddp_comp` array but mtk_drm_crtc_create() never seemed to
> initialize it in the test case I ran. For me, this caused a later
> crash when we looped through the array in mtk_drm_crtc_mode_valid().
> This showed up for me when I booted with `slub_debug=FZPUA` which
> poisons the memory initially. Without `slub_debug` I couldn't
> reproduce, presumably because the later code handles the value being
> NULL and in most cases (not guaranteed in all cases) the memory the
> allocator returned started out as 0.
> 
> It really doesn't hurt to initialize the array with devm_kcalloc()
> since the array is small and the overhead of initting a handful of
> elements to 0 is small. In general initting memory to zero is a safer
> practice and usually it's suggested to only use the non-initting
> alloc
> functions if you really need to.
> 
> Let's switch the function to use an allocation function that zeros
> the
> memory. For me, this avoids the crash.

Reviewed-by: CK Hu <ck.hu@...iatek.com>

> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection
> capability")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> I don't have a ton of experience with this driver to know if the fact
> that the array item was still uninitialized when
> mtk_drm_crtc_mode_valid() ran is the sign of a bug that should be
> fixed. However, even if it is a bug and that bug is fixed then
> zeroing
> memory when we allocate is still safer. If it's a bug that this
> memory
> wasn't initialized then please consider this patch a bug report. ;-)
> 
> I'll also note that I reproduced this on a downstream 6.1-based
> kernel. It appears that only mt8188 uses `conn_routes` and, as far as
> I can tell, mt8188 isn't supported upstream yet.
> 
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index a04499c4f9ca..29207b2756c1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -1009,10 +1009,10 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  
>  	mtk_crtc->mmsys_dev = priv->mmsys_dev;
>  	mtk_crtc->ddp_comp_nr = path_len;
> -	mtk_crtc->ddp_comp = devm_kmalloc_array(dev,
> -						mtk_crtc->ddp_comp_nr +
> (conn_routes ? 1 : 0),
> -						sizeof(*mtk_crtc-
> >ddp_comp),
> -						GFP_KERNEL);
> +	mtk_crtc->ddp_comp = devm_kcalloc(dev,
> +					  mtk_crtc->ddp_comp_nr +
> (conn_routes ? 1 : 0),
> +					  sizeof(*mtk_crtc->ddp_comp),
> +					  GFP_KERNEL);
>  	if (!mtk_crtc->ddp_comp)
>  		return -ENOMEM;
>  
> -- 
> 2.44.0.396.g6e790dbe36-goog