[PATCH AUTOSEL 4.4 08/31] drm/virtio: Fixes a potential NULL pointer dereference on probe failure

[PATCH AUTOSEL 4.4 08/31] drm/virtio: Fixes a potential NULL pointer dereference on probe failure

[Sasha Levin]

From: Xie Yongji <xieyongji@bytedance.com>

[ Upstream commit 17f46f488a5d82c5568e6e786cd760bba1c2ee09 ]

The dev->dev_private might not be allocated if virtio_gpu_pci_quirk()
or virtio_gpu_init() failed. In this case, we should avoid the cleanup
in virtio_gpu_release().

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20210517084913.403-1-xieyongji@bytedance.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/gpu/drm/virtio/virtgpu_kms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 476b9993b068..88ed5e235e55 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -257,6 +257,9 @@ int virtio_gpu_driver_unload(struct drm_device *dev)
 	flush_work(&vgdev->config_changed_work);
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
 
+	if (!vgdev)
+		return;
+
 	virtio_gpu_modeset_fini(vgdev);
 	virtio_gpu_ttm_fini(vgdev);
 	virtio_gpu_free_vbufs(vgdev);
-- 
2.30.2

[PATCH AUTOSEL 4.4 09/31] drm/virtio: Fix double free on probe failure

[Sasha Levin]

From: Xie Yongji <xieyongji@bytedance.com>

[ Upstream commit cec7f1774605a5ef47c134af62afe7c75c30b0ee ]

The virtio_gpu_init() will free vgdev and vgdev->vbufs on failure.
But such failure will be caught by virtio_gpu_probe() and then
virtio_gpu_release() will be called to do some cleanup which
will free vgdev and vgdev->vbufs again. So let's set dev->dev_private
to NULL to avoid double free.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20210517084913.403-2-xieyongji@bytedance.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/gpu/drm/virtio/virtgpu_kms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 88ed5e235e55..fcf9b572ec03 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -233,6 +233,7 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned long flags)
 err_vbufs:
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
 err_vqs:
+	dev->dev_private = NULL;
 	kfree(vgdev);
 	return ret;
 }
-- 
2.30.2

Re: [PATCH AUTOSEL 4.4 08/31] drm/virtio: Fixes a potential NULL pointer dereference on probe failure

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

Hi!

> From: Xie Yongji <xieyongji@bytedance.com>
> 
> [ Upstream commit 17f46f488a5d82c5568e6e786cd760bba1c2ee09 ]
> 
> The dev->dev_private might not be allocated if virtio_gpu_pci_quirk()
> or virtio_gpu_init() failed. In this case, we should avoid the cleanup
> in virtio_gpu_release().

The check is in wrong place at least in 4.4:

> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -257,6 +257,9 @@ int virtio_gpu_driver_unload(struct drm_device *dev)
>  	flush_work(&vgdev->config_changed_work);
>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
>  
> +	if (!vgdev)
> +		return;
> +

Pointer is dereferenced before being tested.

Best regards,
								Pavel
--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH AUTOSEL 4.4 08/31] drm/virtio: Fixes a potential NULL pointer dereference on probe failure

[Sasha Levin]

On Mon, Jul 12, 2021 at 11:59:37PM +0200, Pavel Machek wrote:
>Hi!
>
>> From: Xie Yongji <xieyongji@bytedance.com>
>>
>> [ Upstream commit 17f46f488a5d82c5568e6e786cd760bba1c2ee09 ]
>>
>> The dev->dev_private might not be allocated if virtio_gpu_pci_quirk()
>> or virtio_gpu_init() failed. In this case, we should avoid the cleanup
>> in virtio_gpu_release().
>
>The check is in wrong place at least in 4.4:
>
>> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>> @@ -257,6 +257,9 @@ int virtio_gpu_driver_unload(struct drm_device *dev)
>>  	flush_work(&vgdev->config_changed_work);
>>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
>>
>> +	if (!vgdev)
>> +		return;
>> +
>
>Pointer is dereferenced before being tested.

Heh, yes, thanks for catching that. I'll drop it for now and rework it
next week.

-- 
Thanks,
Sasha