Re: [PATCH 13/22] gpu: host1x: Do not leak BO's phys address to userspace

On 13.06.2017 20:31, Mikko Perttunen wrote:

On 05/23/2017 03:14 AM, Dmitry Osipenko wrote:

...

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 digetx@gmail.com

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

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c index 14f3f957ffab..190353944d23 100644 --- a/drivers/gpu/host1x/job.c +++ b/drivers/gpu/host1x/job.c @@ -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)) {
  

•        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

What about this variant?

-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,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; + 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, @@ -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) host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);

return 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) 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!