[PATCH 13/22] gpu: host1x: Do not leak BO's phys address to userspace
Mikko Perttunen
cyndis at kapsi.fi
Tue Jun 13 19:03:28 UTC 2017
On 06/13/2017 09:21 PM, Dmitry Osipenko wrote:
> 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;
This no longer represents the address of a page, so I think it would be
better to just assign the final value to target.
> + 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)
And then we wouldn't need the IS_ENABLED here
> host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
>
> return 0;
>
with that change, looks good to me
>>> }
>>> - 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!
>
More information about the dri-devel
mailing list