Tegra DRM fixes

This fixes an OOPS in case of out-of-bounds accessing of a kmap'ed cmdbuf (non-IOMMU allocation) while patching the relocations in do_relocs().

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/gem.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/tegra/gem.c +3 -1

index 424569b53e57..ca0d4439e97b 100644
@@ -74,6 +74,9 @@ static void *tegra_bo_kmap(struct host1x_bo *bo, unsigned int page)
 {
        struct tegra_bo *obj = host1x_to_tegra_bo(bo);
 
+       if (page * PAGE_SIZE >= obj->gem.size)
+               return NULL;
+
        if (obj->vaddr)
                return obj->vaddr + page * PAGE_SIZE;
        else if (obj->gem.import_attach)
--

2.13.0

This fixes an OOPS in case of out-of-bounds accessing of a kmap'ed cmdbuf (non-IOMMU allocation) while patching the relocations in do_relocs().

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/gem.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/gpu/drm/tegra/gem.c +0 -0

index 424569b53e57..ca0d4439e97b 100644

drivers/gpu/drm/tegra/gem.c +0 -0

  	if (obj->vaddr)
  		return obj->vaddr + page * PAGE_SIZE;
  	else if (obj->gem.import_attach)

Quoted 8 lines · Dmitry Osipenko

This fixes an OOPS in case of out-of-bounds accessing of a kmap'ed cmdbuf (non-IOMMU allocation) while patching the relocations in do_relocs().

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/gem.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/gpu/drm/tegra/gem.c static void *tegra_bo_kmap(struct host1x_bo *bo, unsigned · line 79

Quoted 11 lines

drivers/gpu/drm/tegra/gem.c +2 -0

index 424569b53e57..ca0d4439e97b 100644
@@ -74,6 +74,9 @@ static void *tegra_bo_kmap(struct host1x_bo *bo, unsigned
 int page)
  {
      struct tegra_bo *obj = host1x_to_tegra_bo(bo);
  +    if (page * PAGE_SIZE >= obj->gem.size)
+        return NULL;
+

The multiplication here could overflow, so it needs the same u64 treatment to catch all problem situations. I'm not sure if this is required, though, with the other bounds check patches in this series.

Quoted 4 lines

drivers/gpu/drm/tegra/gem.c +0 -0

      if (obj->vaddr)
          return obj->vaddr + page * PAGE_SIZE;
      else if (obj->gem.import_attach)

If commands buffer claims a number of words that is higher than its BO can fit, a kernel OOPS will be fired on the out-of-bounds BO access. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.

The CMDA commands buffer address is 4 bytes aligned, so check its alignment.

The maximum number of the CDMA gather fetches is 16383, add a check for it.

Add a sanity check for the relocations in a same way.

[   46.829393] Unable to handle kernel paging request at virtual address f09b2000

...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
  drivers/gpu/drm/tegra/drm.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
  drivers/gpu/drm/tegra/gem.c |  5 -----
  drivers/gpu/drm/tegra/gem.h |  5 +++++
  3 files changed, 53 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +12 -0

index f9282da94a1f..7e4559ec824d 100644
@@ -26,6 +26,7 @@
  #define DRIVER_PATCHLEVEL 0
 
  #define CARVEOUT_SZ SZ_64M
+#define CDMA_GATHER_FETCHES_MAX_NB 16383
 
  struct tegra_drm_file {
  	struct idr contexts;

@@ -383,18 +384,41 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  	while (num_cmdbufs) {
  		struct drm_tegra_cmdbuf cmdbuf;
  		struct host1x_bo *bo;
+		struct tegra_bo *obj;
+		u64 offset;
 
  		if (copy_from_user(&cmdbuf, cmdbufs, sizeof(cmdbuf))) {
  			err = -EFAULT;
  			goto fail;
  		}
 
+		/*
+		 * The maximum number of CDMA gather fetches is 16383, a higher
+		 * value means the words count is malformed.
+		 */
+		if (cmdbuf.words > CDMA_GATHER_FETCHES_MAX_NB) {
+			err = -EINVAL;
+			goto fail;
+		}
+
  		bo = host1x_bo_lookup(file, cmdbuf.handle);

drivers/gpu/drm/tegra/drm.c +10 -0

+		 * is malformed.
+		 */
+		if (offset & 3 || offset >= obj->gem.size) {
+			err = -EINVAL;
+			goto fail;
+		}
+
  		host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
  		num_cmdbufs--;
  		cmdbufs++;

@@ -402,11 +426,35 @@ int tegra_drm_submit(struct tegra_drm_context *context,
 
  	/* copy and resolve relocations from submit */
  	while (num_relocs--) {
+		struct host1x_reloc *reloc;
+		struct tegra_bo *obj;
+
  		err = host1x_reloc_copy_from_user(&job->relocarray[num_relocs],

drivers/gpu/drm/tegra/drm.c +15 -0

+		 * during of the relocations patching, corrupting the commands
+		 * stream.
+		 */
+		if (reloc->cmdbuf.offset & 3 ||
+		    reloc->cmdbuf.offset >= obj->gem.size) {
+			err = -EINVAL;
+			goto fail;
+		}
+
+		obj = host1x_to_tegra_bo(reloc->target.bo);
+
+		if (reloc->target.offset >= obj->gem.size) {
+			err = -EINVAL;
+			goto fail;
+		}
  	}
 
  	if (copy_from_user(job->waitchk, waitchks,

drivers/gpu/drm/tegra/gem.c +0 -5

index ca0d4439e97b..6a855b2f07fb 100644
@@ -20,11 +20,6 @@
  #include "drm.h"
  #include "gem.h"
 
-static inline struct tegra_bo *host1x_to_tegra_bo(struct host1x_bo *bo)
-{
-	return container_of(bo, struct tegra_bo, base);
-}
-
  static void tegra_bo_put(struct host1x_bo *bo)
  {
  	struct tegra_bo *obj = host1x_to_tegra_bo(bo);

drivers/gpu/drm/tegra/gem.h +5 -0

index 6c5f12ac0087..8b32a6fd586d 100644
@@ -52,6 +52,11 @@ static inline struct tegra_bo *to_tegra_bo(struct drm_gem_object *gem)
  	return container_of(gem, struct tegra_bo, gem);
  }
 
+static inline struct tegra_bo *host1x_to_tegra_bo(struct host1x_bo *bo)
+{
+	return container_of(bo, struct tegra_bo, base);
+}
+
  struct tegra_bo *tegra_bo_create(struct drm_device *drm, size_t size,
  				 unsigned long flags);
  struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,

The waitchecks along with multiple syncpoints per submit are not ready for use yet, let's forbid them for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/drm.c | 60 ++++++++++++++++++++++++++++++++++++++++++---
  drivers/gpu/host1x/job.h    |  7 ------
  include/linux/host1x.h      |  7 ++++++
  3 files changed, 63 insertions(+), 11 deletions(-)

drivers/gpu/drm/tegra/drm.c +56 -4

index 7e4559ec824d..eae0c1512ab0 100644
@@ -349,6 +349,36 @@ static int host1x_reloc_copy_from_user(struct host1x_reloc *dest,
  	return 0;
  }
 
+static int host1x_waitchk_copy_from_user(struct host1x_waitchk *dest,
+					 struct drm_tegra_waitchk __user *src,
+					 struct drm_file *file)
+{
+	u32 cmdbuf;
+	int err;
+
+	err = get_user(cmdbuf, &src->handle);
+	if (err < 0)
+		return err;
+
+	err = get_user(dest->offset, &src->offset);
+	if (err < 0)
+		return err;
+
+	err = get_user(dest->syncpt_id, &src->syncpt);
+	if (err < 0)
+		return err;
+
+	err = get_user(dest->thresh, &src->thresh);
+	if (err < 0)
+		return err;
+
+	dest->bo = host1x_bo_lookup(file, cmdbuf);
+	if (!dest->bo)
+		return -ENOENT;
+
+	return 0;
+}
+
  int tegra_drm_submit(struct tegra_drm_context *context,
  		     struct drm_tegra_submit *args, struct drm_device *drm,
  		     struct drm_file *file)

@@ -370,6 +400,10 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  	if (args->num_syncpts != 1)
  		return -EINVAL;
 
+	/* We don't yet support waitchks */
+	if (args->num_waitchks != 0)
+		return -EINVAL;
+
  	job = host1x_job_alloc(context->channel, args->num_cmdbufs,
  			       args->num_relocs, args->num_waitchks);
  	if (!job)

@@ -457,10 +491,28 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  		}
  	}
 
-	if (copy_from_user(job->waitchk, waitchks,
-			   sizeof(*waitchks) * num_waitchks)) {
-		err = -EFAULT;
-		goto fail;
+	/* copy and resolve waitchks from submit */
+	while (num_waitchks--) {
+		struct host1x_waitchk *wait = &job->waitchk[num_waitchks];
+		struct tegra_bo *obj;
+
+		err = host1x_waitchk_copy_from_user(wait,
+						    &waitchks[num_waitchks],
+						    file);
+		if (err < 0)
+			goto fail;
+
+		obj = host1x_to_tegra_bo(wait->bo);
+
+		/*
+		 * The unaligned offset will cause an unaligned write during
+		 * of the waitchks patching, corrupting the commands stream.
+		 */
+		if (wait->offset & 3 ||
+		    wait->offset >= obj->gem.size) {
+			err = -EINVAL;
+			goto fail;
+		}
  	}
 
  	if (copy_from_user(&syncpt, (void __user *)(uintptr_t)args->syncpts,

drivers/gpu/host1x/job.h +0 -7

index 878239c476d2..0debd93a1849 100644
@@ -34,13 +34,6 @@ struct host1x_cmdbuf {
  	u32 pad;
  };
 
-struct host1x_waitchk {
-	struct host1x_bo *bo;
-	u32 offset;
-	u32 syncpt_id;
-	u32 thresh;
-};
-
  struct host1x_job_unpin_data {
  	struct host1x_bo *bo;
  	struct sg_table *sgt;

include/linux/host1x.h +7 -0

index 3d04aa1dc83e..aa323e43ae4e 100644
@@ -177,6 +177,13 @@ struct host1x_reloc {
  	unsigned long shift;
  };
 
+struct host1x_waitchk {
+	struct host1x_bo *bo;
+	u32 offset;
+	u32 syncpt_id;
+	u32 thresh;
+};
+
  struct host1x_job {
  	/* When refcount goes to zero, job can be freed */
  	struct kref ref;

In case of invalid syncpoint ID, the host1x_syncpt_get() returns NULL and none of its users perform a check of the returned pointer later. Let's bail out until it's too late.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/drm.c | 9 +++++++++
  1 file changed, 9 insertions(+)

drivers/gpu/drm/tegra/drm.c +9 -0

index eae0c1512ab0..cdb05d6efde4 100644
@@ -393,6 +393,8 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  	struct drm_tegra_waitchk __user *waitchks =
  		(void __user *)(uintptr_t)args->waitchks;
  	struct drm_tegra_syncpt syncpt;
+	struct host1x *host1x = dev_get_drvdata(drm->dev->parent);
+	struct host1x_syncpt *sp;
  	struct host1x_job *job;
  	int err;
 

@@ -521,6 +523,13 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  		goto fail;
  	}
 
+	/* check whether syncpoint ID is valid */
+	sp = host1x_syncpt_get(host1x, syncpt.id);
+	if (!sp) {
+		err = -ENOENT;
+		goto fail;
+	}
+
  	job->is_addr_reg = context->client->ops->is_addr_reg;
  	job->syncpt_incrs = syncpt.incrs;
  	job->syncpt_id = syncpt.id;

The framebuffers console fbcon_startup() increments the tegra_drm module 'use' refcount via try_module_get(), causing an interlock of the DRM subsys and the tegra_drm modules. In result, the tegra_drm module can't be unloaded using rmmod.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/fb.c | 1 -
 1 file changed, 1 deletion(-)

Quoted 9 lines · Dmitry Osipenko

The framebuffers console fbcon_startup() increments the tegra_drm module 'use' refcount via try_module_get(), causing an interlock of the DRM subsys and the tegra_drm modules. In result, the tegra_drm module can't be unloaded using rmmod.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/fb.c | 1 -
 1 file changed, 1 deletion(-)

That's done on purpose because otherwise you could just rip out the driver from under the framebuffer emulation and things would crash.

My understanding is that the right way to unload a module is to unbind the driver first (which will cause the framebuffer to be removed and hence the reference to be dropped) before the rmmod.

Thierry

Quoted 18 lines · Thierry Reding

Quoted 9 lines · Dmitry Osipenko

The framebuffers console fbcon_startup() increments the tegra_drm module 'use' refcount via try_module_get(), causing an interlock of the DRM subsys and the tegra_drm modules. In result, the tegra_drm module can't be unloaded using rmmod.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/fb.c | 1 -
 1 file changed, 1 deletion(-)

That's done on purpose because otherwise you could just rip out the driver from under the framebuffer emulation and things would crash.

My understanding is that the right way to unload a module is to unbind the driver first (which will cause the framebuffer to be removed and hence the reference to be dropped) before the rmmod.

Quoted 2 lines

Thierry

Aha, interesting. I'll try the unbinding and will drop this patch from the series, thank you for the clarification. I haven't observed any crashes on a module reloading (framebuffer detached/attached just fine), maybe I was lucky then.

Quoted 19 lines · Dmitry Osipenko

Quoted 18 lines · Thierry Reding

Quoted 9 lines · Dmitry Osipenko

The framebuffers console fbcon_startup() increments the tegra_drm module 'use' refcount via try_module_get(), causing an interlock of the DRM subsys and the tegra_drm modules. In result, the tegra_drm module can't be unloaded using rmmod.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/fb.c | 1 -
 1 file changed, 1 deletion(-)

That's done on purpose because otherwise you could just rip out the driver from under the framebuffer emulation and things would crash.

My understanding is that the right way to unload a module is to unbind the driver first (which will cause the framebuffer to be removed and hence the reference to be dropped) before the rmmod.

Quoted 6 lines

Quoted 2 lines

Thierry

Aha, interesting. I'll try the unbinding and will drop this patch from the series, thank you for the clarification. I haven't observed any crashes on a module reloading (framebuffer detached/attached just fine), maybe I was lucky then.

It's possible that it works by accident. The driver removes the framebuffer as part of the driver removal process, so technically nothing should crash. However, if, for any reason, anyone was holding on to a reference to the framebuffer (not sure if that is even possible) the module needs to stay around long enough as well, otherwise the function pointers would become dangling. The module reference makes sure that this doesn't happen (as long as a framebuffer exists, the ops will stick around).

So even if there isn't a way to make it crash today, the code is still correct in taking the reference.

Commit 33a8eb8 ("Implement runtime PM") introduced HW reset control. It causes a hang on Tegra20 if both display controllers are utilized (RGB panel and HDMI). The TRM suggests that each display controller has its own reset control, apparently it is not correct. Let's remove the interaction with the resets for now as a workaround.

Fixes 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/dc.c | 15 ---------------
 1 file changed, 15 deletions(-)

Quoted 11 lines · Dmitry Osipenko

Commit 33a8eb8 ("Implement runtime PM") introduced HW reset control. It causes a hang on Tegra20 if both display controllers are utilized (RGB panel and HDMI). The TRM suggests that each display controller has its own reset control, apparently it is not correct. Let's remove the interaction with the resets for now as a workaround.

Fixes 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/dc.c | 15 ---------------
 1 file changed, 15 deletions(-)

Do you mind if I parameterize this to restrict omission of the assert and deassert to Tegra20? I'm fairly sure that these resets are important on later chips in order to properly reset the display controllers.

My proposal would be to add a field to struct tegra_dc_soc_info (maybe .broken_reset) that will be set only on Tegra20 to still allow the DC to be reset on later generations.

Quoted 13 lines · Thierry Reding

Quoted 11 lines · Dmitry Osipenko

Commit 33a8eb8 ("Implement runtime PM") introduced HW reset control. It causes a hang on Tegra20 if both display controllers are utilized (RGB panel and HDMI). The TRM suggests that each display controller has its own reset control, apparently it is not correct. Let's remove the interaction with the resets for now as a workaround.

Fixes 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/dc.c | 15 ---------------
 1 file changed, 15 deletions(-)

Quoted 3 lines

Do you mind if I parameterize this to restrict omission of the assert and deassert to Tegra20? I'm fairly sure that these resets are important on later chips in order to properly reset the display controllers.

I don't mind at all, I can parameterize it in the next rev myself if you wish.

In case of relocations / waitchecks patching failure the jobs pins stay referenced till DRM file get closed, wasting memory. Add the missed unpinning.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

drivers/gpu/host1x/job.c +7 -10

index d9933828fe87..14f3f957ffab 100644
@@ -592,22 +592,20 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 
                err = do_relocs(job, g->bo);
                if (err)
-                       break;
+                       goto out;
 
                err = do_waitchks(job, host, g->bo);
                if (err)
-                       break;
+                       goto out;
        }
 
-       if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && !err) {
-               err = copy_gathers(job, dev);
-               if (err) {
-                       host1x_job_unpin(job);
-                       return err;
-               }
-       }
+       if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+               goto out;
 
+       err = copy_gathers(job, dev);
 out:
+       if (err)
+               host1x_job_unpin(job);
        wmb();
 
        return err;
--

2.13.0

In case of relocations / waitchecks patching failure the jobs pins stay referenced till DRM file get closed, wasting memory. Add the missed unpinning.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/host1x/job.c | 16 +++++++---------
  1 file changed, 7 insertions(+), 9 deletions(-)

drivers/gpu/host1x/job.c +7 -9

index d9933828fe87..14f3f957ffab 100644
@@ -592,22 +592,20 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
 
  		err = do_relocs(job, g->bo);
  		if (err)
-			break;
+			goto out;
 
  		err = do_waitchks(job, host, g->bo);
  		if (err)
-			break;
+			goto out;
  	}
 
-	if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && !err) {
-		err = copy_gathers(job, dev);
-		if (err) {
-			host1x_job_unpin(job);
-			return err;
-		}
-	}
+	if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+		goto out;
 
+	err = copy_gathers(job, dev);
  out:
+	if (err)
+		host1x_job_unpin(job);
  	wmb();
 
  	return err;

Do gathers coping before patching them, so the original gathers are left untouched. That's not as bad as leaking a kernel addresses, but still doesn't feel right.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/host1x/job.c | 44 ++++++++++++++++++++++++++++++--------------
  1 file changed, 30 insertions(+), 14 deletions(-)

drivers/gpu/host1x/job.c +9 -4

index 14f3f957ffab..190353944d23 100644
@@ -137,8 +137,9 @@ static void host1x_syncpt_patch_offset(struct host1x_syncpt *sp,
   * avoid a wrap condition in the HW).
   */
  static int do_waitchks(struct host1x_job *job, struct host1x *host,
-		       struct host1x_bo *patch)
+		       struct host1x_job_gather *g)
  {
+	struct host1x_bo *patch = g->bo;
  	int i;
 
  	/* compare syncpt vs wait threshold */

@@ -165,7 +166,8 @@ static int do_waitchks(struct host1x_job *job, struct host1x *host,
  				wait->syncpt_id, sp->name, wait->thresh,
  				host1x_syncpt_read_min(sp));
 
-			host1x_syncpt_patch_offset(sp, patch, wait->offset);
+			host1x_syncpt_patch_offset(sp, patch,
+						   g->offset + wait->offset);
  		}
 
  		wait->bo = NULL;

@@ -269,11 +271,12 @@ static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
  	return err;
  }
 
-static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
  {
  	int i = 0;
  	u32 last_page = ~0;
  	void *cmdbuf_page_addr = NULL;
+	struct host1x_bo *cmdbuf = g->bo;
 
  	/* pin & patch the relocs for one gather */
  	for (i = 0; i < job->num_relocs; i++) {

@@ -286,7 +289,8 @@ static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
  		if (cmdbuf != reloc->cmdbuf.bo)
  			continue;
 
-		if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
+		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) &&
+		    last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
  			if (cmdbuf_page_addr)
  				host1x_bo_kunmap(cmdbuf, last_page,
  						 cmdbuf_page_addr);

@@ -301,11 +305,20 @@ static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
  			}
  		}
 
+		if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {

drivers/gpu/host1x/job.c +7 -1

  	}
 
-	if (cmdbuf_page_addr)
+	if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && cmdbuf_page_addr)
  		host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
 
  	return 0;

@@ -573,6 +586,12 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
  	if (err)
  		goto out;
 
+	if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
+		err = copy_gathers(job, dev);
+		if (err)
+			goto out;
+	}
+
  	/* patch gathers */
  	for (i = 0; i < job->num_gathers; i++) {
  		struct host1x_job_gather *g = &job->gathers[i];

drivers/gpu/host1x/job.c +4 -8

  		for (j = i + 1; j < job->num_gathers; j++) {
  			if (job->gathers[j].bo == g->bo) {

@@ -590,19 +610,15 @@ int host1x_job_pin(struct host1x_job *job, struct device *dev)
  			}
  		}
 
-		err = do_relocs(job, g->bo);
+		err = do_relocs(job, g);
  		if (err)
-			goto out;
+			break;
 
-		err = do_waitchks(job, host, g->bo);
+		err = do_waitchks(job, host, g);
  		if (err)
-			goto out;
+			break;
  	}
 
-	if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
-		goto out;
-
-	err = copy_gathers(job, dev);
  out:
  	if (err)
  		host1x_job_unpin(job);

Quoted 61 lines · Dmitry Osipenko

Do gathers coping before patching them, so the original gathers are left untouched. That's not as bad as leaking a kernel addresses, but still doesn't feel right.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/host1x/job.c | 44 ++++++++++++++++++++++++++++++--------------
  1 file changed, 30 insertions(+), 14 deletions(-)

drivers/gpu/host1x/job.c +8 -1

index 14f3f957ffab..190353944d23 100644
@@ -137,8 +137,9 @@ static void host1x_syncpt_patch_offset(struct
 host1x_syncpt *sp,
   * avoid a wrap condition in the HW).
   */
  static int do_waitchks(struct host1x_job *job, struct host1x *host,
-               struct host1x_bo *patch)
+               struct host1x_job_gather *g)
  {
+    struct host1x_bo *patch = g->bo;
      int i;
        /* compare syncpt vs wait threshold */

@@ -165,7 +166,8 @@ static int do_waitchks(struct host1x_job *job, struct
 host1x *host,
                  wait->syncpt_id, sp->name, wait->thresh,
                  host1x_syncpt_read_min(sp));
  -            host1x_syncpt_patch_offset(sp, patch, wait->offset);
+            host1x_syncpt_patch_offset(sp, patch,
+                           g->offset + wait->offset);
          }
            wait->bo = NULL;

@@ -269,11 +271,12 @@ static unsigned int pin_job(struct host1x *host, struct
 host1x_job *job)
      return err;
  }
  -static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
  {
      int i = 0;
      u32 last_page = ~0;
      void *cmdbuf_page_addr = NULL;
+    struct host1x_bo *cmdbuf = g->bo;
        /* pin & patch the relocs for one gather */
      for (i = 0; i < job->num_relocs; i++) {

@@ -286,7 +289,8 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
          if (cmdbuf != reloc->cmdbuf.bo)
              continue;
  -        if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) &&
+            last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
              if (cmdbuf_page_addr)
                  host1x_bo_kunmap(cmdbuf, last_page,
                           cmdbuf_page_addr);

@@ -301,11 +305,20 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
              }
          }
  +        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {

drivers/gpu/host1x/job.c static int do_relocs(struct host1x_job *job, struct · line 318

Quoted 10 lines

drivers/gpu/host1x/job.c +8 -0

+            cmdbuf_page_addr = job->gather_copy_mapped;
+            cmdbuf_page_addr += g->offset;
+        }
+
          target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
+
+        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            target += (reloc->cmdbuf.offset & PAGE_MASK) >> 2;
+
          *target = reloc_addr;

I think this logic would be cleaner like ..

if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
target = job->gather_copy_mapped + g->offset + reloc->cmdbuf.offset;
...
} else {

  if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
    ...

}
target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
}
*target = reloc_addr

Quoted 19 lines

drivers/gpu/host1x/job.c +6 -0

      }
  -    if (cmdbuf_page_addr)
+    if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && cmdbuf_page_addr)
          host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
        return 0;

@@ -573,6 +586,12 @@ int host1x_job_pin(struct host1x_job *job, struct device
 *dev)
      if (err)
          goto out;
  +    if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
+        err = copy_gathers(job, dev);
+        if (err)
+            goto out;
+    }
+
      /* patch gathers */
      for (i = 0; i < job->num_gathers; i++) {
          struct host1x_job_gather *g = &job->gathers[i];

@@ -581,7 +600,8 @@ int host1x_job_pin(struct host1x_job *job, struct device

drivers/gpu/host1x/job.c int host1x_job_pin(struct host1x_job *job, struct device *dev) · line 604

Quoted 6 lines

drivers/gpu/host1x/job.c +2 -0

 *dev)
          if (g->handled)
              continue;
  -        g->base = job->gather_addr_phys[i];
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            g->base = job->gather_addr_phys[i];

Perhaps add a comment here like "copy_gathers sets base when firewall is enabled"

Quoted 81 lines · Mikko Perttunen

Quoted 61 lines · Dmitry Osipenko

Do gathers coping before patching them, so the original gathers are left untouched. That's not as bad as leaking a kernel addresses, but still doesn't feel right.

Signed-off-by Dmitry Osipenko <[email protected]>

---
   drivers/gpu/host1x/job.c | 44 ++++++++++++++++++++++++++++++--------------
   1 file changed, 30 insertions(+), 14 deletions(-)

drivers/gpu/host1x/job.c +8 -1

index 14f3f957ffab..190353944d23 100644
@@ -137,8 +137,9 @@ static void host1x_syncpt_patch_offset(struct
 host1x_syncpt *sp,
    * avoid a wrap condition in the HW).
    */
   static int do_waitchks(struct host1x_job *job, struct host1x *host,
-               struct host1x_bo *patch)
+               struct host1x_job_gather *g)
   {
+    struct host1x_bo *patch = g->bo;
       int i;
         /* compare syncpt vs wait threshold */

@@ -165,7 +166,8 @@ static int do_waitchks(struct host1x_job *job, struct
 host1x *host,
                   wait->syncpt_id, sp->name, wait->thresh,
                   host1x_syncpt_read_min(sp));
   -            host1x_syncpt_patch_offset(sp, patch, wait->offset);
+            host1x_syncpt_patch_offset(sp, patch,
+                           g->offset + wait->offset);
           }
             wait->bo = NULL;

@@ -269,11 +271,12 @@ static unsigned int pin_job(struct host1x *host, struct
 host1x_job *job)
       return err;
   }
   -static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
   {
       int i = 0;
       u32 last_page = ~0;
       void *cmdbuf_page_addr = NULL;
+    struct host1x_bo *cmdbuf = g->bo;
         /* pin & patch the relocs for one gather */
       for (i = 0; i < job->num_relocs; i++) {

@@ -286,7 +289,8 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
           if (cmdbuf != reloc->cmdbuf.bo)
               continue;
   -        if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) &&
+            last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
               if (cmdbuf_page_addr)
                   host1x_bo_kunmap(cmdbuf, last_page,
                            cmdbuf_page_addr);

@@ -301,11 +305,20 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
               }
           }
   +        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {

drivers/gpu/host1x/job.c static int do_relocs(struct host1x_job *job, struct · line 318

Quoted 10 lines

drivers/gpu/host1x/job.c +8 -0

+            cmdbuf_page_addr = job->gather_copy_mapped;
+            cmdbuf_page_addr += g->offset;
+        }
+
           target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
+
+        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            target += (reloc->cmdbuf.offset & PAGE_MASK) >> 2;
+
           *target = reloc_addr;

I think this logic would be cleaner like ..

if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
   target = job->gather_copy_mapped + g->offset + reloc->cmdbuf.offset;
   ...
} else {
   if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
     ...

Quoted 5 lines

}
target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
}
*target = reloc_addr

What about this variant?

drivers/gpu/host1x/job.c +2 -1

-static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
  {
  	int i = 0;
  	u32 last_page = ~0;
  	void *cmdbuf_page_addr = NULL;
+	struct host1x_bo *cmdbuf = g->bo;
 
  	/* pin & patch the relocs for one gather */
  	for (i = 0; i < job->num_relocs; i++) {

(quoted diff) +4 -0

+			target = cmdbuf_page_addr + reloc->cmdbuf.offset;
+			goto patch_reloc;
+		}
+
  		if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
  			if (cmdbuf_page_addr)

host1x_bo_kunmap(cmdbuf, last_page,

drivers/gpu/host1x/job.c +0 -0

  		host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
 

return 0;

Quoted 19 lines

Quoted 19 lines

drivers/gpu/host1x/job.c +6 -0

       }
   -    if (cmdbuf_page_addr)
+    if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && cmdbuf_page_addr)
           host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
         return 0;

@@ -573,6 +586,12 @@ int host1x_job_pin(struct host1x_job *job, struct device
 *dev)
       if (err)
           goto out;
   +    if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
+        err = copy_gathers(job, dev);
+        if (err)
+            goto out;
+    }
+
       /* patch gathers */
       for (i = 0; i < job->num_gathers; i++) {
           struct host1x_job_gather *g = &job->gathers[i];

@@ -581,7 +600,8 @@ int host1x_job_pin(struct host1x_job *job, struct device

Quoted 9 lines

drivers/gpu/host1x/job.c int host1x_job_pin(struct host1x_job *job, struct device *dev) · line 604

Quoted 6 lines

drivers/gpu/host1x/job.c +2 -0

 *dev)
           if (g->handled)
               continue;
   -        g->base = job->gather_addr_phys[i];
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            g->base = job->gather_addr_phys[i];

Perhaps add a comment here like "copy_gathers sets base when firewall is enabled"

Okay, added. Thank you for the review comments!

Quoted 101 lines · Dmitry Osipenko

Quoted 81 lines · Mikko Perttunen

Quoted 61 lines · Dmitry Osipenko

Do gathers coping before patching them, so the original gathers are left untouched. That's not as bad as leaking a kernel addresses, but still doesn't feel right.

Signed-off-by Dmitry Osipenko <[email protected]>

---
   drivers/gpu/host1x/job.c | 44 ++++++++++++++++++++++++++++++--------------
   1 file changed, 30 insertions(+), 14 deletions(-)

drivers/gpu/host1x/job.c +8 -1

index 14f3f957ffab..190353944d23 100644
@@ -137,8 +137,9 @@ static void host1x_syncpt_patch_offset(struct
 host1x_syncpt *sp,
    * avoid a wrap condition in the HW).
    */
   static int do_waitchks(struct host1x_job *job, struct host1x *host,
-               struct host1x_bo *patch)
+               struct host1x_job_gather *g)
   {
+    struct host1x_bo *patch = g->bo;
       int i;
         /* compare syncpt vs wait threshold */

@@ -165,7 +166,8 @@ static int do_waitchks(struct host1x_job *job, struct
 host1x *host,
                   wait->syncpt_id, sp->name, wait->thresh,
                   host1x_syncpt_read_min(sp));
   -            host1x_syncpt_patch_offset(sp, patch, wait->offset);
+            host1x_syncpt_patch_offset(sp, patch,
+                           g->offset + wait->offset);
           }
             wait->bo = NULL;

@@ -269,11 +271,12 @@ static unsigned int pin_job(struct host1x *host, struct
 host1x_job *job)
       return err;
   }
   -static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
   {
       int i = 0;
       u32 last_page = ~0;
       void *cmdbuf_page_addr = NULL;
+    struct host1x_bo *cmdbuf = g->bo;
         /* pin & patch the relocs for one gather */
       for (i = 0; i < job->num_relocs; i++) {

@@ -286,7 +289,8 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
           if (cmdbuf != reloc->cmdbuf.bo)
               continue;
   -        if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) &&
+            last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
               if (cmdbuf_page_addr)
                   host1x_bo_kunmap(cmdbuf, last_page,
                            cmdbuf_page_addr);

@@ -301,11 +305,20 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
               }
           }
   +        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {

drivers/gpu/host1x/job.c static int do_relocs(struct host1x_job *job, struct · line 318

Quoted 10 lines

drivers/gpu/host1x/job.c +8 -0

+            cmdbuf_page_addr = job->gather_copy_mapped;
+            cmdbuf_page_addr += g->offset;
+        }
+
           target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
+
+        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            target += (reloc->cmdbuf.offset & PAGE_MASK) >> 2;
+
           *target = reloc_addr;

I think this logic would be cleaner like ..

if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
   target = job->gather_copy_mapped + g->offset + reloc->cmdbuf.offset;
   ...
} else {
   if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
     ...

Quoted 5 lines

}
target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
}
*target = reloc_addr

What about this variant?

(quoted diff) +2 -1

-static int do_relocs(struct host1x_job *job, struct host1x_bo *cmdbuf)
+static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
  {
      int i = 0;
      u32 last_page = ~0;
      void *cmdbuf_page_addr = NULL;
+    struct host1x_bo *cmdbuf = g->bo;
 
      /* pin & patch the relocs for one gather */
      for (i = 0; i < job->num_relocs; i++) {

Review static int do_relocs(struct host1x_job *job, struct · line 294

Quoted 6 lines

(quoted diff) +2 -0

@@ -286,6 +289,12 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
          if (cmdbuf != reloc->cmdbuf.bo)
              continue;
 
+        if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
+            cmdbuf_page_addr = job->gather_copy_mapped + g->offset;

This no longer represents the address of a page, so I think it would be better to just assign the final value to target.

Quoted 8 lines

(quoted diff) +4 -0

+            target = cmdbuf_page_addr + reloc->cmdbuf.offset;
+            goto patch_reloc;
+        }
+
          if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
              if (cmdbuf_page_addr)

host1x_bo_kunmap(cmdbuf, last_page,

Review static int do_relocs(struct host1x_job *job, struct · line 319

Quoted 10 lines

(quoted diff) +2 -1

@@ -302,10 +311,11 @@ static int do_relocs(struct host1x_job *job, struct
 host1x_bo *cmdbuf)
          }
 
          target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
+patch_reloc:
          *target = reloc_addr;
      }
 
-    if (cmdbuf_page_addr)
+    if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && cmdbuf_page_addr)

And then we wouldn't need the IS_ENABLED here

Quoted 4 lines

(quoted diff) +0 -0

          host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
 

      return 0;

with that change, looks good to me

Quoted 32 lines

Quoted 20 lines

Quoted 20 lines

drivers/gpu/host1x/job.c +6 -0

       }
   -    if (cmdbuf_page_addr)
+    if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && cmdbuf_page_addr)
           host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
         return 0;

@@ -573,6 +586,12 @@ int host1x_job_pin(struct host1x_job *job, struct device
 *dev)
       if (err)
           goto out;
   +    if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL)) {
+        err = copy_gathers(job, dev);
+        if (err)
+            goto out;
+    }
+
       /* patch gathers */
       for (i = 0; i < job->num_gathers; i++) {
           struct host1x_job_gather *g = &job->gathers[i];

@@ -581,7 +600,8 @@ int host1x_job_pin(struct host1x_job *job, struct device
 *dev)

Quoted 9 lines

drivers/gpu/host1x/job.c int host1x_job_pin(struct host1x_job *job, struct device · line 605

Quoted 5 lines

drivers/gpu/host1x/job.c +2 -0

           if (g->handled)
               continue;
   -        g->base = job->gather_addr_phys[i];
+        if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
+            g->base = job->gather_addr_phys[i];

Perhaps add a comment here like "copy_gathers sets base when firewall is enabled"

Okay, added. Thank you for the review comments!

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -1

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
        if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
                return false;
 
+       /* relocation shift value validation isn't implemented yet */
+       if (reloc->shift)
+               return false;
+
        return true;
 }
 
--

2.13.0

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -0

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 	if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
 		return false;
 
+	/* relocation shift value validation isn't implemented yet */
+	if (reloc->shift)
+		return false;
+
 	return true;
 }
 

Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), and they are required for VIC job submissions.

Quoted 28 lines

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -0

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
         return false;
 
+    /* relocation shift value validation isn't implemented yet */
+    if (reloc->shift)
+        return false;
+
     return true;
 }

Quoted 3 lines · Mikko Perttunen

Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), and they are required for VIC job submissions.

The *validation* is unimplemented. Since VIC uses the shifts, we may add a validation for it in a way it is done for the registers / class checks, i.e. compare the per address register shift value. I suppose those registers are documented somewhere(TRM), could you please point me to them (TRM page, reg name)?

Quoted 30 lines · Dmitry Osipenko

Quoted 28 lines

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -0

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
         return false;
 
+    /* relocation shift value validation isn't implemented yet */
+    if (reloc->shift)
+        return false;
+
     return true;
 }

Quoted 10 lines · Dmitry Osipenko

Quoted 3 lines · Mikko Perttunen

Reloc shifts are implemented (see assignment of u32 reloc_addr in do_relocs), and they are required for VIC job submissions.

The *validation* is unimplemented. Since VIC uses the shifts, we may add a validation for it in a way it is done for the registers / class checks, i.e. compare the per address register shift value. I suppose those registers are documented somewhere(TRM), could you please point me to them (TRM page, reg name)?

Ah, indeed, sorry. I'm not sure if it's worth implementing validation for e.g. VIC, since it would be quite a bit of work and require many per-chip and per-unit tables or ranges in the kernel. I think it's a better solution to just forbid everything in the firewall for VIC (and eventually other units), since on systems with those units the normal configuration will be IOMMU anyway.

Anyway, For VIC, everything happens using two registers (method index and method parameter), so you would need to first detect the method index, save that, then wait for a method parameter write and then check that against the written method. The TRM currently has a list methods, but doesn't list their parameter - at least most ones taking a pointer are called *_OFFSET, but not sure if all. In any case, I don't think it's worth implementing.

Quoted 32 lines

Quoted 30 lines · Dmitry Osipenko

Quoted 28 lines

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/host1x/job.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -0

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
         return false;
 
+    /* relocation shift value validation isn't implemented yet */
+    if (reloc->shift)
+        return false;
+
     return true;
 }

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work.

Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/host1x/job.c | 4 ++++
  1 file changed, 4 insertions(+)

drivers/gpu/host1x/job.c +4 -0

index 190353944d23..1a1568e64ba8 100644
@@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
  	if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
  		return false;
 
+	/* relocation shift value validation isn't implemented yet */
+	if (reloc->shift)
+		return false;
+
  	return true;
  }
 

Reviewed-by Mikko Perttunen <[email protected]>

Quoted 4 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work.

Perhaps change to "As of now there is no use for the address shifting on Tegra20" if you post another revision.

Quoted 8 lines · Mikko Perttunen

Reviewed-by Mikko Perttunen <[email protected]>

Quoted 4 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work.

Quoted 3 lines

Perhaps change to "As of now there is no use for the address shifting on Tegra20" if you post another revision.

I'll post a new revision of the series after getting comments to the all patches, to not churn the ML. Thank you very much for the reviews!

Quoted 12 lines · Dmitry Osipenko

Quoted 11 lines · Mikko Perttunen

Reviewed-by Mikko Perttunen <[email protected]>

Quoted 4 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work.

Perhaps change to "As of now there is no use for the address shifting on Tegra20" if you post another revision.

Quoted 3 lines

I'll post a new revision of the series after getting comments to the all patches, to not churn the ML. Thank you very much for the reviews!

However, given your previous comments to this patch, I'll probably add a bypass of the shit checking in case of IOMMU presence.

Quoted 13 lines · Dmitry Osipenko

Quoted 12 lines · Dmitry Osipenko

Quoted 11 lines · Mikko Perttunen

Reviewed-by Mikko Perttunen <[email protected]>

Quoted 4 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work.

Perhaps change to "As of now there is no use for the address shifting on Tegra20" if you post another revision.

Quoted 7 lines

Quoted 3 lines

I'll post a new revision of the series after getting comments to the all patches, to not churn the ML. Thank you very much for the reviews!

However, given your previous comments to this patch, I'll probably add a bypass of the shit checking in case of IOMMU presence.

I don't think that's needed - the firewall will deny pretty much all VIC submissions due to is_addr_reg not being implemented so it cannot reasonably be used on modern Tegras anyway.

The RESTART opcode terminates the gather and restarts the CDMA fetching from a specified word << 2 relative to the CDMA start address. That shouldn't be allowed to be done by userspace.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
  drivers/gpu/host1x/job.c | 1 -
  1 file changed, 1 deletion(-)

drivers/gpu/host1x/job.c +0 -1

index 1a1568e64ba8..cf335c5979e2 100644
@@ -497,7 +497,6 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
  				goto out;
  			break;
  		case 4:
-		case 5:
  		case 14:
  			break;
  		default:

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c  |  1 +
 drivers/gpu/drm/tegra/drm.h  |  1 +
 drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
 drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
 include/linux/host1x.h       |  5 ++++-
 5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
        }
 
        job->is_addr_reg = context->client->ops->is_addr_reg;
+       job->is_valid_class = context->client->ops->is_valid_class;
        job->syncpt_incrs = syncpt.incrs;
        job->syncpt_id = syncpt.id;
        job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
                            struct tegra_drm_context *context);
        void (*close_channel)(struct tegra_drm_context *context);
        int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+       int (*is_valid_class)(u32 class);
        int (*submit)(struct tegra_drm_context *context,
                      struct drm_tegra_submit *args, struct drm_device *drm,
                      struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +12 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 offset)
        return 0;
 }
 
+static int gr2d_is_valid_class(u32 class)
+{
+       switch (class) {
+       case HOST1X_CLASS_GR2D:
+       case HOST1X_CLASS_GR2D_SB:
+               return 1;
+       }
+
+       return 0;
+}
+
 static const struct tegra_drm_client_ops gr2d_ops = {
        .open_channel = gr2d_open_channel,
        .close_channel = gr2d_close_channel,
        .is_addr_reg = gr2d_is_addr_reg,
+       .is_valid_class = gr2d_is_valid_class,
        .submit = tegra_drm_submit,
 };
 

drivers/gpu/host1x/job.c +20 -4

index cf335c5979e2..65e12219405a 100644
@@ -358,6 +358,9 @@ struct host1x_firewall {
 
 static int check_register(struct host1x_firewall *fw, unsigned long offset)
 {
+       if (!fw->job->is_addr_reg)
+               return 0;
+
        if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
                if (!fw->num_relocs)
                        return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
        return 0;
 }
 
+static int check_class(struct host1x_firewall *fw, u32 class)
+{
+       if (!fw->job->is_valid_class) {
+               if (fw->class != class)
+                       return -EINVAL;
+       } else {
+               if (!fw->job->is_valid_class(fw->class))
+                       return -EINVAL;
+       }
+
+       return 0;
+}
+
 static int check_mask(struct host1x_firewall *fw)
 {
        u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
        u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
                (g->offset / sizeof(u32));
+       u32 job_class = fw->class;
        int err = 0;
 
-       if (!fw->job->is_addr_reg)
-               return 0;
-
        fw->words = g->words;
        fw->cmdbuf = g->bo;
        fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
                        fw->class = word >> 6 & 0x3ff;
                        fw->mask = word & 0x3f;
                        fw->reg = word >> 16 & 0xfff;
-                       err = check_mask(fw);
+                       err = check_class(fw, job_class);
+                       if (!err)
+                               err = check_mask(fw);
                        if (err)
                                goto out;
                        break;

include/linux/host1x.h +0 -0

index aa323e43ae4e..561d6bb6580d 100644

include/linux/host1x.h +3 -1

+
+       /* Check if class belongs to the unit */
+       int (*is_valid_class)(u32 class);
 
        /* Request a SETCLASS to this class */
        u32 class;
--

2.13.0

Quoted 132 lines · Dmitry Osipenko

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c  |  1 +
 drivers/gpu/drm/tegra/drm.h  |  1 +
 drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
 drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
 include/linux/host1x.h       |  5 ++++-
 5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
        }
 
        job->is_addr_reg = context->client->ops->is_addr_reg;
+       job->is_valid_class = context->client->ops->is_valid_class;
        job->syncpt_incrs = syncpt.incrs;
        job->syncpt_id = syncpt.id;
        job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
                            struct tegra_drm_context *context);
        void (*close_channel)(struct tegra_drm_context *context);
        int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+       int (*is_valid_class)(u32 class);
        int (*submit)(struct tegra_drm_context *context,
                      struct drm_tegra_submit *args, struct drm_device *drm,
                      struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +12 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 offset)
        return 0;
 }
 
+static int gr2d_is_valid_class(u32 class)
+{
+       switch (class) {
+       case HOST1X_CLASS_GR2D:
+       case HOST1X_CLASS_GR2D_SB:
+               return 1;
+       }
+
+       return 0;
+}
+
 static const struct tegra_drm_client_ops gr2d_ops = {
        .open_channel = gr2d_open_channel,
        .close_channel = gr2d_close_channel,
        .is_addr_reg = gr2d_is_addr_reg,
+       .is_valid_class = gr2d_is_valid_class,
        .submit = tegra_drm_submit,
 };
 

drivers/gpu/host1x/job.c +20 -4

index cf335c5979e2..65e12219405a 100644
@@ -358,6 +358,9 @@ struct host1x_firewall {
 
 static int check_register(struct host1x_firewall *fw, unsigned long offset)
 {
+       if (!fw->job->is_addr_reg)
+               return 0;
+
        if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
                if (!fw->num_relocs)
                        return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
        return 0;
 }
 
+static int check_class(struct host1x_firewall *fw, u32 class)
+{
+       if (!fw->job->is_valid_class) {
+               if (fw->class != class)
+                       return -EINVAL;
+       } else {
+               if (!fw->job->is_valid_class(fw->class))
+                       return -EINVAL;
+       }
+
+       return 0;
+}
+
 static int check_mask(struct host1x_firewall *fw)
 {
        u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
        u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
                (g->offset / sizeof(u32));
+       u32 job_class = fw->class;
        int err = 0;
 
-       if (!fw->job->is_addr_reg)
-               return 0;
-
        fw->words = g->words;
        fw->cmdbuf = g->bo;
        fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
                        fw->class = word >> 6 & 0x3ff;
                        fw->mask = word & 0x3f;
                        fw->reg = word >> 16 & 0xfff;
-                       err = check_mask(fw);
+                       err = check_class(fw, job_class);
+                       if (!err)
+                               err = check_mask(fw);
                        if (err)
                                goto out;
                        break;

include/linux/host1x.h +0 -0

index aa323e43ae4e..561d6bb6580d 100644
@@ -233,7 +233,10 @@ struct host1x_job {
        u8 *gather_copy_mapped;
 

include/linux/host1x.h struct host1x_job { · line 236

Quoted 3 lines

include/linux/host1x.h +1 -1

        /* Check if register is marked as an address reg */
-       int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
+       int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);

This seems like an unrelated fix, you might want to mention it in the commit message at least.

Quoted 132 lines · Dmitry Osipenko

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c  |  1 +
 drivers/gpu/drm/tegra/drm.h  |  1 +
 drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
 drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
 include/linux/host1x.h       |  5 ++++-
 5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
        }
 
        job->is_addr_reg = context->client->ops->is_addr_reg;
+       job->is_valid_class = context->client->ops->is_valid_class;
        job->syncpt_incrs = syncpt.incrs;
        job->syncpt_id = syncpt.id;
        job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
                            struct tegra_drm_context *context);
        void (*close_channel)(struct tegra_drm_context *context);
        int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+       int (*is_valid_class)(u32 class);
        int (*submit)(struct tegra_drm_context *context,
                      struct drm_tegra_submit *args, struct drm_device *drm,
                      struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +12 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 offset)
        return 0;
 }
 
+static int gr2d_is_valid_class(u32 class)
+{
+       switch (class) {
+       case HOST1X_CLASS_GR2D:
+       case HOST1X_CLASS_GR2D_SB:
+               return 1;
+       }
+
+       return 0;
+}
+
 static const struct tegra_drm_client_ops gr2d_ops = {
        .open_channel = gr2d_open_channel,
        .close_channel = gr2d_close_channel,
        .is_addr_reg = gr2d_is_addr_reg,
+       .is_valid_class = gr2d_is_valid_class,
        .submit = tegra_drm_submit,
 };
 

drivers/gpu/host1x/job.c +20 -4

index cf335c5979e2..65e12219405a 100644
@@ -358,6 +358,9 @@ struct host1x_firewall {
 
 static int check_register(struct host1x_firewall *fw, unsigned long offset)
 {
+       if (!fw->job->is_addr_reg)
+               return 0;
+
        if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
                if (!fw->num_relocs)
                        return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
        return 0;
 }
 
+static int check_class(struct host1x_firewall *fw, u32 class)
+{
+       if (!fw->job->is_valid_class) {
+               if (fw->class != class)
+                       return -EINVAL;
+       } else {
+               if (!fw->job->is_valid_class(fw->class))
+                       return -EINVAL;
+       }
+
+       return 0;
+}
+
 static int check_mask(struct host1x_firewall *fw)
 {
        u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
        u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
                (g->offset / sizeof(u32));
+       u32 job_class = fw->class;
        int err = 0;
 
-       if (!fw->job->is_addr_reg)
-               return 0;
-
        fw->words = g->words;
        fw->cmdbuf = g->bo;
        fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
                        fw->class = word >> 6 & 0x3ff;
                        fw->mask = word & 0x3f;
                        fw->reg = word >> 16 & 0xfff;
-                       err = check_mask(fw);
+                       err = check_class(fw, job_class);
+                       if (!err)
+                               err = check_mask(fw);
                        if (err)
                                goto out;
                        break;

include/linux/host1x.h +0 -0

index aa323e43ae4e..561d6bb6580d 100644
@@ -233,7 +233,10 @@ struct host1x_job {
        u8 *gather_copy_mapped;
 

include/linux/host1x.h struct host1x_job { · line 236

Quoted 3 lines

include/linux/host1x.h +1 -1

        /* Check if register is marked as an address reg */
-       int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
+       int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);

This seems like an unrelated fix, you might want to mention it in the commit message at least.

Quoted 133 lines · Erik Faye-Lund

Quoted 132 lines · Dmitry Osipenko

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c  |  1 +
 drivers/gpu/drm/tegra/drm.h  |  1 +
 drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
 drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
 include/linux/host1x.h       |  5 ++++-
 5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
        }
 
        job->is_addr_reg = context->client->ops->is_addr_reg;
+       job->is_valid_class = context->client->ops->is_valid_class;
        job->syncpt_incrs = syncpt.incrs;
        job->syncpt_id = syncpt.id;
        job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
                            struct tegra_drm_context *context);
        void (*close_channel)(struct tegra_drm_context *context);
        int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+       int (*is_valid_class)(u32 class);
        int (*submit)(struct tegra_drm_context *context,
                      struct drm_tegra_submit *args, struct drm_device *drm,
                      struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +12 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 offset)
        return 0;
 }
 
+static int gr2d_is_valid_class(u32 class)
+{
+       switch (class) {
+       case HOST1X_CLASS_GR2D:
+       case HOST1X_CLASS_GR2D_SB:
+               return 1;
+       }
+
+       return 0;
+}
+
 static const struct tegra_drm_client_ops gr2d_ops = {
        .open_channel = gr2d_open_channel,
        .close_channel = gr2d_close_channel,
        .is_addr_reg = gr2d_is_addr_reg,
+       .is_valid_class = gr2d_is_valid_class,
        .submit = tegra_drm_submit,
 };
 

drivers/gpu/host1x/job.c +20 -4

index cf335c5979e2..65e12219405a 100644
@@ -358,6 +358,9 @@ struct host1x_firewall {
 
 static int check_register(struct host1x_firewall *fw, unsigned long offset)
 {
+       if (!fw->job->is_addr_reg)
+               return 0;
+
        if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
                if (!fw->num_relocs)
                        return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
        return 0;
 }
 
+static int check_class(struct host1x_firewall *fw, u32 class)
+{
+       if (!fw->job->is_valid_class) {
+               if (fw->class != class)
+                       return -EINVAL;
+       } else {
+               if (!fw->job->is_valid_class(fw->class))
+                       return -EINVAL;
+       }
+
+       return 0;
+}
+
 static int check_mask(struct host1x_firewall *fw)
 {
        u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
 {
        u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
                (g->offset / sizeof(u32));
+       u32 job_class = fw->class;
        int err = 0;
 
-       if (!fw->job->is_addr_reg)
-               return 0;
-
        fw->words = g->words;
        fw->cmdbuf = g->bo;
        fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
                        fw->class = word >> 6 & 0x3ff;
                        fw->mask = word & 0x3f;
                        fw->reg = word >> 16 & 0xfff;
-                       err = check_mask(fw);
+                       err = check_class(fw, job_class);
+                       if (!err)
+                               err = check_mask(fw);
                        if (err)
                                goto out;
                        break;

include/linux/host1x.h +0 -0

index aa323e43ae4e..561d6bb6580d 100644
@@ -233,7 +233,10 @@ struct host1x_job {
        u8 *gather_copy_mapped;
 

Quoted 6 lines

include/linux/host1x.h struct host1x_job { · line 236

Quoted 3 lines

include/linux/host1x.h +1 -1

        /* Check if register is marked as an address reg */
-       int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
+       int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);

This seems like an unrelated fix, you might want to mention it in the commit message at least.

If you're going to rev the series anyway, might be worth splitting this off into a separate commit to make it stand out more.

Either way is fine with me.

Thierry

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/drm.c  |  1 +
  drivers/gpu/drm/tegra/drm.h  |  1 +
  drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
  drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
  include/linux/host1x.h       |  5 ++++-
  5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
  	}
 
  	job->is_addr_reg = context->client->ops->is_addr_reg;
+	job->is_valid_class = context->client->ops->is_valid_class;
  	job->syncpt_incrs = syncpt.incrs;
  	job->syncpt_id = syncpt.id;
  	job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
  			    struct tegra_drm_context *context);
  	void (*close_channel)(struct tegra_drm_context *context);
  	int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+	int (*is_valid_class)(u32 class);
  	int (*submit)(struct tegra_drm_context *context,
  		      struct drm_tegra_submit *args, struct drm_device *drm,
  		      struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +0 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32 class, u32 offset)
  	return 0;
  }
 

drivers/gpu/drm/tegra/gr2d.c +2 -0

+
  static const struct tegra_drm_client_ops gr2d_ops = {
  	.open_channel = gr2d_open_channel,
  	.close_channel = gr2d_close_channel,
  	.is_addr_reg = gr2d_is_addr_reg,
+	.is_valid_class = gr2d_is_valid_class,
  	.submit = tegra_drm_submit,
  };
 

drivers/gpu/host1x/job.c +20 -4

index cf335c5979e2..65e12219405a 100644
@@ -358,6 +358,9 @@ struct host1x_firewall {
 
  static int check_register(struct host1x_firewall *fw, unsigned long offset)
  {
+	if (!fw->job->is_addr_reg)
+		return 0;
+
  	if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
  		if (!fw->num_relocs)
  			return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
  	return 0;
  }
 
+static int check_class(struct host1x_firewall *fw, u32 class)
+{
+	if (!fw->job->is_valid_class) {
+		if (fw->class != class)
+			return -EINVAL;
+	} else {
+		if (!fw->job->is_valid_class(fw->class))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
  static int check_mask(struct host1x_firewall *fw)
  {
  	u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
  {
  	u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
  		(g->offset / sizeof(u32));
+	u32 job_class = fw->class;
  	int err = 0;
 
-	if (!fw->job->is_addr_reg)
-		return 0;
-
  	fw->words = g->words;
  	fw->cmdbuf = g->bo;
  	fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct host1x_job_gather *g)
  			fw->class = word >> 6 & 0x3ff;
  			fw->mask = word & 0x3f;
  			fw->reg = word >> 16 & 0xfff;
-			err = check_mask(fw);
+			err = check_class(fw, job_class);
+			if (!err)
+				err = check_mask(fw);
  			if (err)
  				goto out;
  			break;

include/linux/host1x.h +4 -1

index aa323e43ae4e..561d6bb6580d 100644
@@ -233,7 +233,10 @@ struct host1x_job {
  	u8 *gather_copy_mapped;
 
  	/* Check if register is marked as an address reg */
-	int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
+	int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);
+
+	/* Check if class belongs to the unit */
+	int (*is_valid_class)(u32 class);
 
  	/* Request a SETCLASS to this class */
  	u32 class;

With the simplification below and the is_addr_reg change mentioned,

Reviewed-by Mikko Perttunen <[email protected]>

Quoted 47 lines · Dmitry Osipenko

Several channels could be made to write the same unit concurrently via the SETCLASS opcode, trusting userspace is a bad idea. It should be possible to drop the per-client channel reservation and add a per-unit locking by inserting MLOCK's to the command stream to re-allow the SETCLASS opcode, but it will be much more work. Let's forbid the unit-unrelated class changes for now.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/gpu/drm/tegra/drm.c  |  1 +
  drivers/gpu/drm/tegra/drm.h  |  1 +
  drivers/gpu/drm/tegra/gr2d.c | 12 ++++++++++++
  drivers/gpu/host1x/job.c     | 24 ++++++++++++++++++++----
  include/linux/host1x.h       |  5 ++++-
  5 files changed, 38 insertions(+), 5 deletions(-)

drivers/gpu/drm/tegra/drm.c +1 -0

index cdb05d6efde4..17416e1c219a 100644
@@ -531,6 +531,7 @@ int tegra_drm_submit(struct tegra_drm_context *context,
      }
        job->is_addr_reg = context->client->ops->is_addr_reg;
+    job->is_valid_class = context->client->ops->is_valid_class;
      job->syncpt_incrs = syncpt.incrs;
      job->syncpt_id = syncpt.id;
      job->timeout = 10000;

drivers/gpu/drm/tegra/drm.h +1 -0

index 85aa2e3d9d4e..6d6da01282f3 100644
@@ -83,6 +83,7 @@ struct tegra_drm_client_ops {
                  struct tegra_drm_context *context);
      void (*close_channel)(struct tegra_drm_context *context);
      int (*is_addr_reg)(struct device *dev, u32 class, u32 offset);
+    int (*is_valid_class)(u32 class);
      int (*submit)(struct tegra_drm_context *context,
                struct drm_tegra_submit *args, struct drm_device *drm,
                struct drm_file *file);

drivers/gpu/drm/tegra/gr2d.c +0 -0

index 02cd3e37a6ec..782231c41a1a 100644
@@ -109,10 +109,22 @@ static int gr2d_is_addr_reg(struct device *dev, u32
 class, u32 offset)
      return 0;
  }

drivers/gpu/drm/tegra/gr2d.c static int gr2d_is_addr_reg(struct device *dev, u32 · line 121

Quoted 10 lines

drivers/gpu/drm/tegra/gr2d.c +9 -0

  +static int gr2d_is_valid_class(u32 class)
+{
+    switch (class) {
+    case HOST1X_CLASS_GR2D:
+    case HOST1X_CLASS_GR2D_SB:
+        return 1;
+    }
+
+    return 0;
+}

This could be just

return (class == HOST1X_CLASS_GR2D || class == HOST1X_CLASS_GR2D_SB);

Quoted 81 lines

drivers/gpu/drm/tegra/gr2d.c +2 -0

+
  static const struct tegra_drm_client_ops gr2d_ops = {
      .open_channel = gr2d_open_channel,
      .close_channel = gr2d_close_channel,
      .is_addr_reg = gr2d_is_addr_reg,
+    .is_valid_class = gr2d_is_valid_class,
      .submit = tegra_drm_submit,
  };
  diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c

index cf335c5979e2..65e12219405a 100644

drivers/gpu/host1x/job.c +19 -3

@@ -358,6 +358,9 @@ struct host1x_firewall {
    static int check_register(struct host1x_firewall *fw, unsigned long offset)
  {
+    if (!fw->job->is_addr_reg)
+        return 0;
+
      if (fw->job->is_addr_reg(fw->dev, fw->class, offset)) {
          if (!fw->num_relocs)
              return -EINVAL;

@@ -372,6 +375,19 @@ static int check_register(struct host1x_firewall *fw,
 unsigned long offset)
      return 0;
  }
  +static int check_class(struct host1x_firewall *fw, u32 class)
+{
+    if (!fw->job->is_valid_class) {
+        if (fw->class != class)
+            return -EINVAL;
+    } else {
+        if (!fw->job->is_valid_class(fw->class))
+            return -EINVAL;
+    }
+
+    return 0;
+}
+
  static int check_mask(struct host1x_firewall *fw)
  {
      u32 mask = fw->mask;

@@ -445,11 +461,9 @@ static int validate(struct host1x_firewall *fw, struct
 host1x_job_gather *g)
  {
      u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped +
          (g->offset / sizeof(u32));
+    u32 job_class = fw->class;
      int err = 0;
  -    if (!fw->job->is_addr_reg)
-        return 0;
-
      fw->words = g->words;
      fw->cmdbuf = g->bo;
      fw->offset = 0;

@@ -469,7 +483,9 @@ static int validate(struct host1x_firewall *fw, struct
 host1x_job_gather *g)
              fw->class = word >> 6 & 0x3ff;
              fw->mask = word & 0x3f;
              fw->reg = word >> 16 & 0xfff;
-            err = check_mask(fw);
+            err = check_class(fw, job_class);
+            if (!err)
+                err = check_mask(fw);
              if (err)
                  goto out;

              break;

include/linux/host1x.h +4 -1

index aa323e43ae4e..561d6bb6580d 100644
@@ -233,7 +233,10 @@ struct host1x_job {
      u8 *gather_copy_mapped;
        /* Check if register is marked as an address reg */
-    int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
+    int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);
+
+    /* Check if class belongs to the unit */
+    int (*is_valid_class)(u32 class);
        /* Request a SETCLASS to this class */
      u32 class;