[PATCH 46/48] staging: etnaviv: rewrite submit interface to use copy from user
Lucas Stach
l.stach at pengutronix.de
Fri Sep 25 04:57:58 PDT 2015
This rewrites the submit interface to copy the command stream from user
memory. This mitigates a potential attack vector of the old interface
where userspace could submit a command buffer that would be validated by
the kernel, but is still mapped into userspace. This could be exploited
by changing the command stream after validation but before the actual
GPU execution.
A nice side effect is that validation and reloc patching can now operate
on cached memory and only the final result is copied to a writecombined
command buffer which should make those operations a bit more efficient.
A simplification to the interface is the removal of the ability to push
multiple command buffers per submit. As we don't use it for context
restore buffers and the fact that userspace doesn't need to work with
a fixed command buffer size anymore, with the potential risk to overflow
its size in the middle of an atomic stream section, there is no need
for this complication anymore.
Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
---
drivers/staging/etnaviv/etnaviv_buffer.c | 48 +++++------
drivers/staging/etnaviv/etnaviv_cmd_parser.c | 6 +-
drivers/staging/etnaviv/etnaviv_drv.h | 4 +-
drivers/staging/etnaviv/etnaviv_gem.h | 7 +-
drivers/staging/etnaviv/etnaviv_gem_submit.c | 122 ++++++++++-----------------
drivers/staging/etnaviv/etnaviv_gpu.c | 38 +++++++++
drivers/staging/etnaviv/etnaviv_gpu.h | 20 +++++
include/uapi/drm/etnaviv_drm.h | 18 ++--
8 files changed, 136 insertions(+), 127 deletions(-)
diff --git a/drivers/staging/etnaviv/etnaviv_buffer.c b/drivers/staging/etnaviv/etnaviv_buffer.c
index 1994eceae1ae..76c646076b05 100644
--- a/drivers/staging/etnaviv/etnaviv_buffer.c
+++ b/drivers/staging/etnaviv/etnaviv_buffer.c
@@ -111,6 +111,11 @@ static u32 gpu_va(struct etnaviv_gpu *gpu, struct etnaviv_gem_object *obj)
return obj->paddr - gpu->memory_base;
}
+static u32 gpu_va_raw(struct etnaviv_gpu *gpu, u32 paddr)
+{
+ return paddr - gpu->memory_base;
+}
+
static void etnaviv_buffer_dump(struct etnaviv_gpu *gpu,
struct etnaviv_gem_object *obj, u32 off, u32 len)
{
@@ -153,10 +158,9 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
struct etnaviv_gem_submit *submit)
{
struct etnaviv_gem_object *buffer = to_etnaviv_bo(gpu->buffer);
- struct etnaviv_gem_object *cmd;
u32 *lw = buffer->vaddr + ((buffer->offset - 4) * 4);
+ u32 *usercmd = submit->cmdbuf->vaddr;
u32 back, link_target, link_size, reserve_size, extra_size = 0;
- u32 i;
if (drm_debug & DRM_UT_DRIVER)
etnaviv_buffer_dump(gpu, buffer, 0, 0x50);
@@ -194,35 +198,24 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
/* Skip over any extra instructions */
link_target += extra_size * sizeof(u32);
- /* update offset for every cmd stream */
- for (i = submit->nr_cmds; i--; ) {
- cmd = submit->cmd[i].obj;
+ if (drm_debug & DRM_UT_DRIVER)
+ pr_info("stream link to 0x%08x @ 0x%08x %p\n",
+ link_target, gpu_va_raw(gpu, submit->cmdbuf->paddr),
+ submit->cmdbuf->vaddr);
- cmd->offset = submit->cmd[i].offset + submit->cmd[i].size;
+ /* jump back from cmd to main buffer */
+ usercmd[submit->cmdbuf->user_size/4] = VIV_FE_LINK_HEADER_OP_LINK |
+ VIV_FE_LINK_HEADER_PREFETCH(link_size);
+ usercmd[submit->cmdbuf->user_size/4 + 1] = link_target;
- if (drm_debug & DRM_UT_DRIVER)
- pr_info("stream link from buffer %u to 0x%08x @ 0x%08x %p\n",
- i, link_target,
- gpu_va(gpu, cmd) + cmd->offset * 4,
- cmd->vaddr + cmd->offset * 4);
+ link_target = gpu_va_raw(gpu, submit->cmdbuf->paddr);
+ link_size = submit->cmdbuf->size / 8;
- /* jump back from last cmd to main buffer */
- CMD_LINK(cmd, link_size, link_target);
- /* update the size */
- submit->cmd[i].size = cmd->offset - submit->cmd[i].offset;
-
- link_target = gpu_va(gpu, cmd) + submit->cmd[i].offset * 4;
- link_size = submit->cmd[i].size * 2;
- }
if (drm_debug & DRM_UT_DRIVER) {
- for (i = 0; i < submit->nr_cmds; i++) {
- struct etnaviv_gem_object *obj = submit->cmd[i].obj;
-
- etnaviv_buffer_dump(gpu, obj, submit->cmd[i].offset,
- submit->cmd[i].size);
- }
+ print_hex_dump(KERN_INFO, "cmd ", DUMP_PREFIX_OFFSET, 16, 4,
+ submit->cmdbuf->vaddr, submit->cmdbuf->size, 0);
pr_info("link op: %p\n", lw);
pr_info("link addr: %p\n", lw + 1);
@@ -263,6 +256,11 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
/* Save the event and buffer position of the new event trigger */
gpu->event[event].fence = submit->fence;
+ /* take ownership of cmdbuffer*/
+ submit->cmdbuf->fence = submit->fence;
+ list_add_tail(&submit->cmdbuf->gpu_active_list, &gpu->active_cmd_list);
+ submit->cmdbuf = NULL;
+
/* trigger event */
CMD_LOAD_STATE(buffer, VIVS_GL_EVENT, VIVS_GL_EVENT_EVENT_ID(event) |
VIVS_GL_EVENT_FROM_PE);
diff --git a/drivers/staging/etnaviv/etnaviv_cmd_parser.c b/drivers/staging/etnaviv/etnaviv_cmd_parser.c
index 7ae6ddb4306e..5175d6eb3bdc 100644
--- a/drivers/staging/etnaviv/etnaviv_cmd_parser.c
+++ b/drivers/staging/etnaviv/etnaviv_cmd_parser.c
@@ -61,10 +61,10 @@ static uint8_t cmd_length[32] = {
[FE_OPCODE_STALL] = 2,
};
-bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu,
- struct etnaviv_gem_object *obj, unsigned int offset, unsigned int size)
+bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu, void *stream,
+ unsigned int size)
{
- u32 *start = obj->vaddr + offset * 4;
+ u32 *start = stream;
u32 *buf = start;
u32 *end = buf + size;
diff --git a/drivers/staging/etnaviv/etnaviv_drv.h b/drivers/staging/etnaviv/etnaviv_drv.h
index 40629207b04f..2b4b87bdda18 100644
--- a/drivers/staging/etnaviv/etnaviv_drv.h
+++ b/drivers/staging/etnaviv/etnaviv_drv.h
@@ -99,6 +99,8 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj);
void etnaviv_gem_free_object(struct drm_gem_object *obj);
int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
u32 size, u32 flags, u32 *handle);
+struct drm_gem_object *etnaviv_gem_new_locked(struct drm_device *dev,
+ u32 size, u32 flags);
struct drm_gem_object *etnaviv_gem_new(struct drm_device *dev,
u32 size, u32 flags);
int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
@@ -108,7 +110,7 @@ void etnaviv_buffer_end(struct etnaviv_gpu *gpu);
void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int event,
struct etnaviv_gem_submit *submit);
bool etnaviv_cmd_validate_one(struct etnaviv_gpu *gpu,
- struct etnaviv_gem_object *obj, unsigned int offset, unsigned int size);
+ void *stream, unsigned int size);
#ifdef CONFIG_DEBUG_FS
void etnaviv_gem_describe_objects(struct list_head *list, struct seq_file *m);
diff --git a/drivers/staging/etnaviv/etnaviv_gem.h b/drivers/staging/etnaviv/etnaviv_gem.h
index 5112c5458306..c991d12e7aed 100644
--- a/drivers/staging/etnaviv/etnaviv_gem.h
+++ b/drivers/staging/etnaviv/etnaviv_gem.h
@@ -118,13 +118,8 @@ struct etnaviv_gem_submit {
struct list_head bo_list;
struct ww_acquire_ctx ticket;
u32 fence;
- unsigned int nr_cmds;
unsigned int nr_bos;
- struct {
- u32 offset; /* in dwords */
- u32 size; /* in dwords */
- struct etnaviv_gem_object *obj;
- } cmd[MAX_CMDS];
+ struct etnaviv_cmdbuf *cmdbuf;
struct {
u32 flags;
struct etnaviv_gem_object *obj;
diff --git a/drivers/staging/etnaviv/etnaviv_gem_submit.c b/drivers/staging/etnaviv/etnaviv_gem_submit.c
index 73c1378e96a1..f886a3c66d30 100644
--- a/drivers/staging/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/staging/etnaviv/etnaviv_gem_submit.c
@@ -45,7 +45,7 @@ static struct etnaviv_gem_submit *submit_create(struct drm_device *dev,
/* initially, until copy_from_user() and bo lookup succeeds: */
submit->nr_bos = 0;
- submit->nr_cmds = 0;
+ submit->cmdbuf = NULL;
INIT_LIST_HEAD(&submit->bo_list);
ww_acquire_init(&submit->ticket, &reservation_ww_class);
@@ -212,11 +212,11 @@ static int submit_bo(struct etnaviv_gem_submit *submit, u32 idx,
}
/* process the reloc's and patch up the cmdstream as needed: */
-static int submit_reloc(struct etnaviv_gem_submit *submit, struct etnaviv_gem_object *obj,
- u32 offset, u32 nr_relocs, u64 relocs)
+static int submit_reloc(struct etnaviv_gem_submit *submit, void *stream,
+ u32 size, u32 nr_relocs, u64 relocs)
{
u32 i, last_offset = 0;
- u32 *ptr = obj->vaddr;
+ u32 *ptr = stream;
int ret;
for (i = 0; i < nr_relocs; i++) {
@@ -240,7 +240,7 @@ static int submit_reloc(struct etnaviv_gem_submit *submit, struct etnaviv_gem_ob
/* offset in dwords: */
off = submit_reloc.submit_offset / 4;
- if ((off >= (obj->base.size / 4)) ||
+ if ((off >= size ) ||
(off < last_offset)) {
DRM_ERROR("invalid offset %u at reloc %u\n", off, i);
return -EINVAL;
@@ -276,6 +276,9 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit, bool fail)
drm_gem_object_unreference(&etnaviv_obj->base);
}
+ if (submit->cmdbuf)
+ etnaviv_gpu_cmdbuf_free(submit->cmdbuf);
+
ww_acquire_fini(&submit->ticket);
kfree(submit);
}
@@ -286,11 +289,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
struct etnaviv_drm_private *priv = dev->dev_private;
struct drm_etnaviv_gem_submit *args = data;
struct etnaviv_file_private *ctx = file->driver_priv;
- struct drm_etnaviv_gem_submit_cmd *cmds;
struct drm_etnaviv_gem_submit_bo *bos;
struct etnaviv_gem_submit *submit;
+ struct etnaviv_cmdbuf *cmdbuf;
struct etnaviv_gpu *gpu;
- unsigned i;
+ void *stream;
int ret;
if (args->pipe >= ETNA_MAX_PIPES)
@@ -300,27 +303,33 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
if (!gpu)
return -ENXIO;
- if (args->nr_cmds > MAX_CMDS)
+ if (args->stream_size % 4) {
+ DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
+ args->stream_size);
return -EINVAL;
+ }
/*
* Copy the command submission and bo array to kernel space in
* one go, and do this outside of the dev->struct_mutex lock.
*/
- cmds = drm_malloc_ab(args->nr_cmds, sizeof(*cmds));
bos = drm_malloc_ab(args->nr_bos, sizeof(*bos));
- if (!cmds || !bos)
- return -ENOMEM;
+ stream = drm_malloc_ab(1, args->stream_size);
+ cmdbuf = etnaviv_gpu_cmdbuf_new(gpu, ALIGN(args->stream_size, 8) + 8);
+ if (!bos || !stream || !cmdbuf) {
+ ret = -ENOMEM;
+ goto err_submit_cmds;
+ }
- ret = copy_from_user(cmds, to_user_ptr(args->cmds),
- args->nr_cmds * sizeof(*cmds));
+ ret = copy_from_user(bos, to_user_ptr(args->bos),
+ args->nr_bos * sizeof(*bos));
if (ret) {
ret = -EFAULT;
goto err_submit_cmds;
}
- ret = copy_from_user(bos, to_user_ptr(args->bos),
- args->nr_bos * sizeof(*bos));
+ ret = copy_from_user(stream, to_user_ptr(args->stream),
+ args->stream_size);
if (ret) {
ret = -EFAULT;
goto err_submit_cmds;
@@ -364,69 +373,21 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
- for (i = 0; i < args->nr_cmds; i++) {
- struct drm_etnaviv_gem_submit_cmd *submit_cmd = cmds + i;
- struct etnaviv_gem_object *etnaviv_obj;
- unsigned max_size;
-
- ret = submit_bo(submit, submit_cmd->submit_idx, &etnaviv_obj,
- NULL);
- if (ret)
- goto out;
-
- if (!(etnaviv_obj->flags & ETNA_BO_CMDSTREAM)) {
- DRM_ERROR("cmdstream bo has flag ETNA_BO_CMDSTREAM not set\n");
- ret = -EINVAL;
- goto out;
- }
-
- if (submit_cmd->size % 4) {
- DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
- submit_cmd->size);
- ret = -EINVAL;
- goto out;
- }
-
- if (submit_cmd->submit_offset % 8) {
- DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
- submit_cmd->size);
- ret = -EINVAL;
- goto out;
- }
-
- /*
- * We must have space to add a LINK command at the end of
- * the command buffer.
- */
- max_size = etnaviv_obj->base.size - 8;
-
- if (submit_cmd->size > max_size ||
- submit_cmd->submit_offset > max_size - submit_cmd->size) {
- DRM_ERROR("invalid cmdstream size: %u\n",
- submit_cmd->size);
- ret = -EINVAL;
- goto out;
- }
-
- submit->cmd[i].offset = submit_cmd->submit_offset / 4;
- submit->cmd[i].size = submit_cmd->size / 4;
- submit->cmd[i].obj = etnaviv_obj;
-
- if (!etnaviv_cmd_validate_one(gpu, etnaviv_obj,
- submit->cmd[i].offset,
- submit->cmd[i].size)) {
- ret = -EINVAL;
- goto out;
- }
-
- ret = submit_reloc(submit, etnaviv_obj,
- submit_cmd->submit_offset,
- submit_cmd->nr_relocs, submit_cmd->relocs);
- if (ret)
- goto out;
+ if (!etnaviv_cmd_validate_one(gpu, stream, args->stream_size / 4)) {
+ ret = -EINVAL;
+ goto out;
}
- submit->nr_cmds = i;
+ ret = submit_reloc(submit, stream, args->stream_size / 4,
+ args->nr_relocs, args->relocs);
+ if (ret)
+ goto out;
+
+ memcpy(cmdbuf->vaddr, stream, args->stream_size);
+ cmdbuf->user_size = ALIGN(args->stream_size, 8);
+ /* transfer ownership of cmdbuf to submit */
+ submit->cmdbuf = cmdbuf;
+ cmdbuf = NULL;
ret = etnaviv_gpu_submit(gpu, submit, ctx);
@@ -447,11 +408,14 @@ out:
if (ret == -EAGAIN)
flush_workqueue(priv->wq);
- err_submit_cmds:
+err_submit_cmds:
+ /* if we still own the cmdbuf */
+ if (cmdbuf)
+ etnaviv_gpu_cmdbuf_free(cmdbuf);
+ if (stream)
+ drm_free_large(stream);
if (bos)
drm_free_large(bos);
- if (cmds)
- drm_free_large(cmds);
return ret;
}
diff --git a/drivers/staging/etnaviv/etnaviv_gpu.c b/drivers/staging/etnaviv/etnaviv_gpu.c
index 39ac08d26260..f8b507406e41 100644
--- a/drivers/staging/etnaviv/etnaviv_gpu.c
+++ b/drivers/staging/etnaviv/etnaviv_gpu.c
@@ -857,12 +857,41 @@ static void event_free(struct etnaviv_gpu *gpu, unsigned int event)
* Cmdstream submission/retirement:
*/
+struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu, u32 size)
+{
+ struct etnaviv_cmdbuf *cmdbuf;
+
+ cmdbuf = kzalloc(sizeof(*cmdbuf), GFP_KERNEL);
+ if (!cmdbuf)
+ return NULL;
+
+ cmdbuf->vaddr = dma_alloc_writecombine(gpu->dev, size, &cmdbuf->paddr,
+ GFP_KERNEL);
+ if (!cmdbuf->vaddr) {
+ kfree(cmdbuf);
+ return NULL;
+ }
+
+ cmdbuf->gpu = gpu;
+ cmdbuf->size = size;
+
+ return cmdbuf;
+}
+
+void etnaviv_gpu_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf)
+{
+ dma_free_writecombine(cmdbuf->gpu->dev, cmdbuf->size,
+ cmdbuf->vaddr, cmdbuf->paddr);
+ kfree(cmdbuf);
+}
+
static void retire_worker(struct work_struct *work)
{
struct etnaviv_gpu *gpu = container_of(work, struct etnaviv_gpu,
retire_work);
struct drm_device *dev = gpu->drm;
u32 fence = gpu->completed_fence;
+ struct etnaviv_cmdbuf *cmdbuf, *tmp;
mutex_lock(&dev->struct_mutex);
@@ -885,6 +914,14 @@ static void retire_worker(struct work_struct *work)
}
}
+ list_for_each_entry_safe(cmdbuf, tmp, &gpu->active_cmd_list,
+ gpu_active_list) {
+ if (fence_after_eq(fence, cmdbuf->fence)) {
+ etnaviv_gpu_cmdbuf_free(cmdbuf);
+ list_del(&cmdbuf->gpu_active_list);
+ }
+ }
+
gpu->retired_fence = fence;
mutex_unlock(&dev->struct_mutex);
@@ -1223,6 +1260,7 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master,
gpu->drm = drm;
INIT_LIST_HEAD(&gpu->active_list);
+ INIT_LIST_HEAD(&gpu->active_cmd_list);
INIT_WORK(&gpu->retire_work, retire_worker);
INIT_WORK(&gpu->recover_work, recover_worker);
init_waitqueue_head(&gpu->fence_event);
diff --git a/drivers/staging/etnaviv/etnaviv_gpu.h b/drivers/staging/etnaviv/etnaviv_gpu.h
index 374f37f3665f..ee1c8504e4c8 100644
--- a/drivers/staging/etnaviv/etnaviv_gpu.h
+++ b/drivers/staging/etnaviv/etnaviv_gpu.h
@@ -103,6 +103,9 @@ struct etnaviv_gpu {
/* list of GEM active objects: */
struct list_head active_list;
+ /* list of currently in-flight command buffers */
+ struct list_head active_cmd_list;
+
u32 idle_mask;
/* Fencing support */
@@ -133,6 +136,20 @@ struct etnaviv_gpu {
struct work_struct recover_work;
};
+struct etnaviv_cmdbuf {
+ /* device this cmdbuf is allocated for */
+ struct etnaviv_gpu *gpu;
+ /* cmdbuf properties */
+ void *vaddr;
+ dma_addr_t paddr;
+ u32 size;
+ u32 user_size;
+ /* fence after which this buffer is to be disposed */
+ u32 fence;
+ /* per GPU in-flight list */
+ struct list_head gpu_active_list;
+};
+
static inline void gpu_write(struct etnaviv_gpu *gpu, u32 reg, u32 data)
{
etnaviv_writel(data, gpu->mmio + reg);
@@ -168,6 +185,9 @@ int etnaviv_gpu_wait_obj_inactive(struct etnaviv_gpu *gpu,
struct etnaviv_gem_object *etnaviv_obj, struct timespec *timeout);
int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
struct etnaviv_gem_submit *submit, struct etnaviv_file_private *ctx);
+struct etnaviv_cmdbuf *etnaviv_gpu_cmdbuf_new(struct etnaviv_gpu *gpu,
+ u32 size);
+void etnaviv_gpu_cmdbuf_free(struct etnaviv_cmdbuf *cmdbuf);
int etnaviv_gpu_pm_get_sync(struct etnaviv_gpu *gpu);
void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu);
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index 1515fa4f6986..689caf93c85d 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -123,15 +123,6 @@ struct drm_etnaviv_gem_submit_reloc {
__u64 reloc_offset; /* in, offset from start of reloc_bo */
};
-struct drm_etnaviv_gem_submit_cmd {
- __u32 submit_idx; /* in, index of submit_bo cmdstream buffer */
- __u32 submit_offset; /* in, offset into submit_bo */
- __u32 size; /* in, cmdstream size */
- __u32 pad;
- __u32 nr_relocs; /* in, number of submit_reloc's */
- __u64 relocs; /* in, ptr to array of submit_reloc's */
-};
-
/* Each buffer referenced elsewhere in the cmdstream submit (ie. the
* cmdstream buffer(s) themselves or reloc entries) has one (and only
* one) entry in the submit->bos[] table.
@@ -159,14 +150,15 @@ struct drm_etnaviv_gem_submit_bo {
#define ETNA_PIPE_2D 0x01
#define ETNA_PIPE_VG 0x02
struct drm_etnaviv_gem_submit {
+ __u32 fence; /* out */
__u32 pipe; /* in */
__u32 exec_state; /* in, initial execution state (ETNA_PIPE_x) */
- __u32 fence; /* out */
__u32 nr_bos; /* in, number of submit_bo's */
- __u32 nr_cmds; /* in, number of submit_cmd's */
- __u32 pad;
+ __u32 nr_relocs; /* in, number of submit_reloc's */
+ __u32 stream_size; /* in, cmdstream size */
__u64 bos; /* in, ptr to array of submit_bo's */
- __u64 cmds; /* in, ptr to array of submit_cmd's */
+ __u64 relocs; /* in, ptr to array of submit_reloc's */
+ __u64 stream; /* in, ptr to cmdstream */
};
/* The normal way to synchronize with the GPU is just to CPU_PREP on
--
2.5.1
More information about the dri-devel
mailing list