[v2] drm/mediatek: Clean dangling pointer on bind error path

  mtk_drm_bind() can fail, in which case drm_dev_put() is called,
destroying the drm_device object. However a pointer to it was still
being held in the private object, and that pointer would be passed along
to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
point, resulting in a panic. Clean the pointer when destroying the
object in the error path to prevent this from happening.

Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

Changes in v2:
- Added Fixes tag

 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
 1 file changed, 1 insertion(+)
  

Il 22/11/22 15:39, Nícolas F. R. A. Prado ha scritto:
> mtk_drm_bind() can fail, in which case drm_dev_put() is called,
> destroying the drm_device object. However a pointer to it was still
> being held in the private object, and that pointer would be passed along
> to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
> point, resulting in a panic. Clean the pointer when destroying the
> object in the error path to prevent this from happening.
> 
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> 
> Changes in v2:
> - Added Fixes tag
> 
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 39a42dc8fb85..a21ff1b3258c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -514,6 +514,7 @@ static int mtk_drm_bind(struct device *dev)
>   err_deinit:
>   	mtk_drm_kms_deinit(drm);
>   err_free:
> +	private->drm = NULL;

Sorry for not noticing that in v1, but I've rechecked this function and, while this
commit does indeed actually solve the described issue, I think it's incomplete.

A few lines before, we have a loop that sets

		private->all_drm_private[i]->drm = drm;

...so here you should do...

	private->drm = NULL;

	while (i--) /* a for loop will also do, your choice */
		private->all_drm_private[i]->drm = NULL;
		
That makes sure that you cleanup *everything* :-)

Cheers,
Angelo
  

On Wed, Nov 23, 2022 at 10:15:25AM +0100, AngeloGioacchino Del Regno wrote:
> Il 22/11/22 15:39, Nícolas F. R. A. Prado ha scritto:
> > mtk_drm_bind() can fail, in which case drm_dev_put() is called,
> > destroying the drm_device object. However a pointer to it was still
> > being held in the private object, and that pointer would be passed along
> > to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
> > point, resulting in a panic. Clean the pointer when destroying the
> > object in the error path to prevent this from happening.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Added Fixes tag
> > 
> >   drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 39a42dc8fb85..a21ff1b3258c 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -514,6 +514,7 @@ static int mtk_drm_bind(struct device *dev)
> >   err_deinit:
> >   	mtk_drm_kms_deinit(drm);
> >   err_free:
> > +	private->drm = NULL;
> 
> Sorry for not noticing that in v1, but I've rechecked this function and, while this
> commit does indeed actually solve the described issue, I think it's incomplete.
> 
> A few lines before, we have a loop that sets
> 
> 		private->all_drm_private[i]->drm = drm;

Actually that line only exists with [1] applied, but that commit hasn't been
merged as of yet. I debated whether it would be better to send this fix as is,
or ask Nancy Lin to add the tweaked fix you mention below to that series, but
sending this fix as is seemed better since it can be backported to older kernel
versions.

So assuming this commit gets merged first, Nancy should make that tweak you
mentioned below to ensure all the pointers are zeroed out, which is why I've
added Nancy to CC. (As a side note, given that only the mmsys with drm_master =
true would ever call the drm suspend helper, even this patch as is is enough to
avoid the panic even with that series applied, still we should zero out all
pointers for good measure)

[1] https://lore.kernel.org/linux-mediatek/20221107072413.16178-6-nancy.lin@mediatek.com/

Thanks,
Nícolas

> 
> ...so here you should do...
> 
> 	private->drm = NULL;
> 
> 	while (i--) /* a for loop will also do, your choice */
> 		private->all_drm_private[i]->drm = NULL;
> 		
> That makes sure that you cleanup *everything* :-)
> 
> Cheers,
> Angelo
>
  

Il 25/11/22 17:34, Nícolas F. R. A. Prado ha scritto:
> On Wed, Nov 23, 2022 at 10:15:25AM +0100, AngeloGioacchino Del Regno wrote:
>> Il 22/11/22 15:39, Nícolas F. R. A. Prado ha scritto:
>>> mtk_drm_bind() can fail, in which case drm_dev_put() is called,
>>> destroying the drm_device object. However a pointer to it was still
>>> being held in the private object, and that pointer would be passed along
>>> to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
>>> point, resulting in a panic. Clean the pointer when destroying the
>>> object in the error path to prevent this from happening.
>>>
>>> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> - Added Fixes tag
>>>
>>>    drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> index 39a42dc8fb85..a21ff1b3258c 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>>> @@ -514,6 +514,7 @@ static int mtk_drm_bind(struct device *dev)
>>>    err_deinit:
>>>    	mtk_drm_kms_deinit(drm);
>>>    err_free:
>>> +	private->drm = NULL;
>>
>> Sorry for not noticing that in v1, but I've rechecked this function and, while this
>> commit does indeed actually solve the described issue, I think it's incomplete.
>>
>> A few lines before, we have a loop that sets
>>
>> 		private->all_drm_private[i]->drm = drm;
> 
> Actually that line only exists with [1] applied, but that commit hasn't been
> merged as of yet. I debated whether it would be better to send this fix as is,
> or ask Nancy Lin to add the tweaked fix you mention below to that series, but
> sending this fix as is seemed better since it can be backported to older kernel
> versions.
> 
> So assuming this commit gets merged first, Nancy should make that tweak you
> mentioned below to ensure all the pointers are zeroed out, which is why I've
> added Nancy to CC. (As a side note, given that only the mmsys with drm_master =
> true would ever call the drm suspend helper, even this patch as is is enough to
> avoid the panic even with that series applied, still we should zero out all
> pointers for good measure)
> 
> [1] https://lore.kernel.org/linux-mediatek/20221107072413.16178-6-nancy.lin@mediatek.com/
> 

Sorry about that, you're right - the proposed fix is for the commit you
linked, I got confused somehow.

This means that you get my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers!
  

Hi, Nicolas:

Nícolas F. R. A. Prado <nfraprado@collabora.com> 於 2022年11月22日 週二 下午10:39寫道：
>
> mtk_drm_bind() can fail, in which case drm_dev_put() is called,
> destroying the drm_device object. However a pointer to it was still
> being held in the private object, and that pointer would be passed along
> to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
> point, resulting in a panic. Clean the pointer when destroying the
> object in the error path to prevent this from happening.

Applied to mediatek-drm-next [1], thanks.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next

Regards,
Chun-Kuang.

>
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> ---
>
> Changes in v2:
> - Added Fixes tag
>
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 39a42dc8fb85..a21ff1b3258c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -514,6 +514,7 @@ static int mtk_drm_bind(struct device *dev)
>  err_deinit:
>         mtk_drm_kms_deinit(drm);
>  err_free:
> +       private->drm = NULL;
>         drm_dev_put(drm);
>         return ret;
>  }
> --
> 2.38.1
>