davidriley at chromium.org
2019-Jun-05 23:44 UTC
[PATCH 1/4] drm/virtio: Ensure cached capset entries are valid before copying.
From: David Riley <davidriley at chromium.org> virtio_gpu_get_caps_ioctl could return success with invalid data if a second caller to the function occurred after the entry was created in virtio_gpu_cmd_get_capset but prior to the virtio_gpu_cmd_capset_cb callback being called. This could leak contents of memory as well since the caps_cache allocation is done without zeroing. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 949a264985fc..88c1ed57a3c5 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -526,7 +526,6 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, list_for_each_entry(cache_ent, &vgdev->cap_cache, head) { if (cache_ent->id == args->cap_set_id && cache_ent->version == args->cap_set_ver) { - ptr = cache_ent->caps_cache; spin_unlock(&vgdev->display_info_lock); goto copy_exit; } @@ -537,6 +536,7 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, virtio_gpu_cmd_get_capset(vgdev, found_valid, args->cap_set_ver, &cache_ent); +copy_exit: ret = wait_event_timeout(vgdev->resp_wq, atomic_read(&cache_ent->is_valid), 5 * HZ); if (!ret) @@ -544,7 +544,6 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, ptr = cache_ent->caps_cache; -copy_exit: if (copy_to_user((void __user *)(unsigned long)args->addr, ptr, size)) return -EFAULT; -- 2.22.0.rc1.311.g5d7573a151-goog
davidriley at chromium.org
2019-Jun-05 23:44 UTC
[PATCH 2/4] drm/virtio: Wake up all waiters when capset response comes in.
From: David Riley <davidriley at chromium.org> If multiple callers occur simultaneously, wake them all up. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_vq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index e62fe24b1a2e..da71568adb9a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -588,7 +588,7 @@ static void virtio_gpu_cmd_capset_cb(struct virtio_gpu_device *vgdev, } } spin_unlock(&vgdev->display_info_lock); - wake_up(&vgdev->resp_wq); + wake_up_all(&vgdev->resp_wq); } static int virtio_get_edid_block(void *data, u8 *buf, -- 2.22.0.rc1.311.g5d7573a151-goog
davidriley at chromium.org
2019-Jun-05 23:44 UTC
[PATCH 3/4] drm/virtio: Fix cache entry creation race.
From: David Riley <davidriley at chromium.org> virtio_gpu_cmd_get_capset would check for the existence of an entry under lock. If it was not found, it would unlock and call virtio_gpu_cmd_get_capset to create a new entry. The new entry would be added it to the list without checking if it was added by another task during the period where the lock was not held resulting in duplicate entries. Compounding this issue, virtio_gpu_cmd_capset_cb would stop iterating after find the first matching entry. Multiple callbacks would modify the first entry, but any subsequent entries and their associated waiters would eventually timeout since they don't become valid, also wasting memory along the way. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_vq.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index da71568adb9a..dd5ead2541c2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -684,8 +684,11 @@ int virtio_gpu_cmd_get_capset(struct virtio_gpu_device *vgdev, struct virtio_gpu_vbuffer *vbuf; int max_size; struct virtio_gpu_drv_cap_cache *cache_ent; + struct virtio_gpu_drv_cap_cache *search_ent; void *resp_buf; + *cache_p = NULL; + if (idx >= vgdev->num_capsets) return -EINVAL; @@ -716,9 +719,26 @@ int virtio_gpu_cmd_get_capset(struct virtio_gpu_device *vgdev, atomic_set(&cache_ent->is_valid, 0); cache_ent->size = max_size; spin_lock(&vgdev->display_info_lock); - list_add_tail(&cache_ent->head, &vgdev->cap_cache); + /* Search while under lock in case it was added by another task. */ + list_for_each_entry(search_ent, &vgdev->cap_cache, head) { + if (search_ent->id == vgdev->capsets[idx].id && + search_ent->version == version) { + *cache_p = search_ent; + break; + } + } + if (!*cache_p) + list_add_tail(&cache_ent->head, &vgdev->cap_cache); spin_unlock(&vgdev->display_info_lock); + if (*cache_p) { + /* Entry was found, so free everything that was just created. */ + kfree(resp_buf); + kfree(cache_ent->caps_cache); + kfree(cache_ent); + return 0; + } + cmd_p = virtio_gpu_alloc_cmd_resp (vgdev, &virtio_gpu_cmd_capset_cb, &vbuf, sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_capset) + max_size, -- 2.22.0.rc1.311.g5d7573a151-goog
davidriley at chromium.org
2019-Jun-05 23:44 UTC
[PATCH 4/4] drm/virtio: Add memory barriers for capset cache.
From: David Riley <davidriley at chromium.org> After data is copied to the cache entry, atomic_set is used indicate that the data is the entry is valid without appropriate memory barriers. Similarly the read side was missing the same memory barries. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +++ drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++ 2 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 88c1ed57a3c5..502f5f7c2298 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -542,6 +542,9 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, if (!ret) return -EBUSY; + /* is_valid check must proceed before copy of the cache entry. */ + virt_rmb(); + ptr = cache_ent->caps_cache; if (copy_to_user((void __user *)(unsigned long)args->addr, ptr, size)) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index dd5ead2541c2..b974eba4fe7d 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -583,6 +583,8 @@ static void virtio_gpu_cmd_capset_cb(struct virtio_gpu_device *vgdev, cache_ent->id == le32_to_cpu(cmd->capset_id)) { memcpy(cache_ent->caps_cache, resp->capset_data, cache_ent->size); + /* Copy must occur before is_valid is signalled. */ + virt_wmb(); atomic_set(&cache_ent->is_valid, 1); break; } -- 2.22.0.rc1.311.g5d7573a151-goog
Gerd Hoffmann
2019-Jun-06 07:41 UTC
[PATCH 4/4] drm/virtio: Add memory barriers for capset cache.
On Wed, Jun 05, 2019 at 04:44:23PM -0700, davidriley at chromium.org wrote:> From: David Riley <davidriley at chromium.org> > > After data is copied to the cache entry, atomic_set is used indicate > that the data is the entry is valid without appropriate memory barriers. > Similarly the read side was missing the same memory barries. > > Signed-off-by: David Riley <davidriley at chromium.org> > --- > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +++ > drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c > index 88c1ed57a3c5..502f5f7c2298 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c > @@ -542,6 +542,9 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, > if (!ret) > return -EBUSY; > > + /* is_valid check must proceed before copy of the cache entry. */ > + virt_rmb();I don't think you need virt_rmb() here. This isn't guest <=> host communication, so a normal barrier should do. The other three fixes are queued up for drm-misc-next. cheers, Gerd
davidriley at chromium.org
2019-Jun-10 21:18 UTC
[PATCH v2 0/4] drm/virtio: Ensure cached capset entries are valid before copying.
From: David Riley <davidriley at chromium.org> v2: Updated barriers. David Riley (4): drm/virtio: Ensure cached capset entries are valid before copying. drm/virtio: Wake up all waiters when capset response comes in. drm/virtio: Fix cache entry creation race. drm/virtio: Add memory barriers for capset cache. drivers/gpu/drm/virtio/virtgpu_ioctl.c | 6 ++++-- drivers/gpu/drm/virtio/virtgpu_vq.c | 26 ++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) -- 2.22.0.rc2.383.gf4fbbf30c2-goog
davidriley at chromium.org
2019-Jun-10 21:18 UTC
[PATCH v2 1/4] drm/virtio: Ensure cached capset entries are valid before copying.
From: David Riley <davidriley at chromium.org> virtio_gpu_get_caps_ioctl could return success with invalid data if a second caller to the function occurred after the entry was created in virtio_gpu_cmd_get_capset but prior to the virtio_gpu_cmd_capset_cb callback being called. This could leak contents of memory as well since the caps_cache allocation is done without zeroing. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 949a264985fc..88c1ed57a3c5 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -526,7 +526,6 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, list_for_each_entry(cache_ent, &vgdev->cap_cache, head) { if (cache_ent->id == args->cap_set_id && cache_ent->version == args->cap_set_ver) { - ptr = cache_ent->caps_cache; spin_unlock(&vgdev->display_info_lock); goto copy_exit; } @@ -537,6 +536,7 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, virtio_gpu_cmd_get_capset(vgdev, found_valid, args->cap_set_ver, &cache_ent); +copy_exit: ret = wait_event_timeout(vgdev->resp_wq, atomic_read(&cache_ent->is_valid), 5 * HZ); if (!ret) @@ -544,7 +544,6 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, ptr = cache_ent->caps_cache; -copy_exit: if (copy_to_user((void __user *)(unsigned long)args->addr, ptr, size)) return -EFAULT; -- 2.22.0.rc2.383.gf4fbbf30c2-goog
davidriley at chromium.org
2019-Jun-10 21:18 UTC
[PATCH v2 2/4] drm/virtio: Wake up all waiters when capset response comes in.
From: David Riley <davidriley at chromium.org> If multiple callers occur simultaneously, wake them all up. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_vq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index e62fe24b1a2e..da71568adb9a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -588,7 +588,7 @@ static void virtio_gpu_cmd_capset_cb(struct virtio_gpu_device *vgdev, } } spin_unlock(&vgdev->display_info_lock); - wake_up(&vgdev->resp_wq); + wake_up_all(&vgdev->resp_wq); } static int virtio_get_edid_block(void *data, u8 *buf, -- 2.22.0.rc2.383.gf4fbbf30c2-goog
davidriley at chromium.org
2019-Jun-10 21:18 UTC
[PATCH v2 3/4] drm/virtio: Fix cache entry creation race.
From: David Riley <davidriley at chromium.org> virtio_gpu_cmd_get_capset would check for the existence of an entry under lock. If it was not found, it would unlock and call virtio_gpu_cmd_get_capset to create a new entry. The new entry would be added it to the list without checking if it was added by another task during the period where the lock was not held resulting in duplicate entries. Compounding this issue, virtio_gpu_cmd_capset_cb would stop iterating after find the first matching entry. Multiple callbacks would modify the first entry, but any subsequent entries and their associated waiters would eventually timeout since they don't become valid, also wasting memory along the way. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_vq.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index da71568adb9a..dd5ead2541c2 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -684,8 +684,11 @@ int virtio_gpu_cmd_get_capset(struct virtio_gpu_device *vgdev, struct virtio_gpu_vbuffer *vbuf; int max_size; struct virtio_gpu_drv_cap_cache *cache_ent; + struct virtio_gpu_drv_cap_cache *search_ent; void *resp_buf; + *cache_p = NULL; + if (idx >= vgdev->num_capsets) return -EINVAL; @@ -716,9 +719,26 @@ int virtio_gpu_cmd_get_capset(struct virtio_gpu_device *vgdev, atomic_set(&cache_ent->is_valid, 0); cache_ent->size = max_size; spin_lock(&vgdev->display_info_lock); - list_add_tail(&cache_ent->head, &vgdev->cap_cache); + /* Search while under lock in case it was added by another task. */ + list_for_each_entry(search_ent, &vgdev->cap_cache, head) { + if (search_ent->id == vgdev->capsets[idx].id && + search_ent->version == version) { + *cache_p = search_ent; + break; + } + } + if (!*cache_p) + list_add_tail(&cache_ent->head, &vgdev->cap_cache); spin_unlock(&vgdev->display_info_lock); + if (*cache_p) { + /* Entry was found, so free everything that was just created. */ + kfree(resp_buf); + kfree(cache_ent->caps_cache); + kfree(cache_ent); + return 0; + } + cmd_p = virtio_gpu_alloc_cmd_resp (vgdev, &virtio_gpu_cmd_capset_cb, &vbuf, sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_capset) + max_size, -- 2.22.0.rc2.383.gf4fbbf30c2-goog
davidriley at chromium.org
2019-Jun-10 21:18 UTC
[PATCH v2 4/4] drm/virtio: Add memory barriers for capset cache.
From: David Riley <davidriley at chromium.org> After data is copied to the cache entry, atomic_set is used indicate that the data is the entry is valid without appropriate memory barriers. Similarly the read side was missing the corresponding memory barriers. Signed-off-by: David Riley <davidriley at chromium.org> --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +++ drivers/gpu/drm/virtio/virtgpu_vq.c | 2 ++ 2 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 88c1ed57a3c5..a50495083d6f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -542,6 +542,9 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev, if (!ret) return -EBUSY; + /* is_valid check must proceed before copy of the cache entry. */ + smp_rmb(); + ptr = cache_ent->caps_cache; if (copy_to_user((void __user *)(unsigned long)args->addr, ptr, size)) diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index dd5ead2541c2..c7a5ca027245 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -583,6 +583,8 @@ static void virtio_gpu_cmd_capset_cb(struct virtio_gpu_device *vgdev, cache_ent->id == le32_to_cpu(cmd->capset_id)) { memcpy(cache_ent->caps_cache, resp->capset_data, cache_ent->size); + /* Copy must occur before is_valid is signalled. */ + smp_wmb(); atomic_set(&cache_ent->is_valid, 1); break; } -- 2.22.0.rc2.383.gf4fbbf30c2-goog
Gerd Hoffmann
2019-Jun-13 06:59 UTC
[PATCH v2 4/4] drm/virtio: Add memory barriers for capset cache.
On Mon, Jun 10, 2019 at 02:18:10PM -0700, davidriley at chromium.org wrote:> From: David Riley <davidriley at chromium.org> > > After data is copied to the cache entry, atomic_set is used indicate > that the data is the entry is valid without appropriate memory barriers. > Similarly the read side was missing the corresponding memory barriers.All pushed to drm-misc-next thanks, Gerd
Possibly Parallel Threads
- [PATCH v2 4/6] virtio-gpu: add 3d/virgl support
- [PATCH v2 4/6] virtio-gpu: add 3d/virgl support
- [PATCH v2 4/4] drm/virtio: Add memory barriers for capset cache.
- [PATCH] drm/virtio: fix bounds check in virtio_gpu_cmd_get_capset()
- [PATCH AUTOSEL 4.14 15/60] drm/virtio: Add memory barriers for capset cache.