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

[Dmitry Osipenko]

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 at 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!

-- 
Dmitry