Hi all,
On 8/21/18 3:19 AM, Thomas Hellstrom wrote:
>>> #include "vmwgfx_drv.h"
>>> #include "vmwgfx_reg.h"
>>> @@ -4520,8 +4521,10 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
>>> unsigned long data,
>>> return -EINVAL;
>>> }
>>>
>>> - if (arg.version > 1 &&
>>> - copy_from_user(&arg.context_handle,
>>> + if (arg.version >= ARRAY_SIZE(copy_offset))
>>> + return -EFAULT;
>
> I must admit my understanding of spectre workings in this case is limited,
> but why do you need to compare
> arg.version against ARRAY_SIZE here, when it is already checked against
> DRM_VMW_EXECBUF_VERSION earlier?
>
Oh, I wasn't aware of the value in DRM_VMW_EXECBUF_VERSION. But as arg.version
is used to index copy_offset,
it is safer to compare its value against the actual size of copy_offset.
So, what do you think if I replace DRM_VMW_EXECBUF_VERSION with ARRAY_SIZE
instead of adding a new check
against ARRAY_SIZE?
Something like:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 1f13457..3ef9f7b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -25,6 +25,7 @@
*
**************************************************************************/
#include <linux/sync_file.h>
+#include <linux/nospec.h>
#include "vmwgfx_drv.h"
#include "vmwgfx_reg.h"
@@ -4514,11 +4515,12 @@ int vmw_execbuf_ioctl(struct drm_device *dev, unsigned
long data,
* arg.version.
*/
- if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
+ if (unlikely(arg.version > ARRAY_SIZE(copy_offset) ||
arg.version == 0)) {
DRM_ERROR("Incorrect execbuf version.\n");
return -EINVAL;
}
+ arg.version = array_index_nospec(arg.version, ARRAY_SIZE(copy_offset));
if (arg.version > 1 &&
copy_from_user(&arg.context_handle,
>
>
>>> + arg.version = array_index_nospec(arg.version,
>>> ARRAY_SIZE(copy_offset));
>>> + if (copy_from_user(&arg.context_handle,
>>> (void __user *) (data + copy_offset[0]),
>>> copy_offset[arg.version - 1] -
>>> copy_offset[0]) != 0)
>
> Similarly, we want to perform this copy iff arg.version > 1. Why did you
> remove that check?
>
Yeah, this check must remain in place. I will add it back and send v2.
Thanks for the feedback!
--
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
