[PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Arto Merilainen]

On 05/26/2013 01:02 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
>> From: Terje Bergstrom 
>>
>> This patch adds several fixes to host1x firewall:
>> - Host1x firewall does not survive if it expects a reloc, but user
>>space didn't pass any relocs. Also it reset the reloc table for
>>each gather, whereas they should be reset only per submit. Also
>>class does not need to be reset for each class - once per submit
>>is enough.
>> - For INCR opcode the check was not working properly at all.
>> - The firewall verified gather buffers before copying them. This
>>allowed a malicious application to rewrite the buffer content by
>>timing the rewrite carefully. This patch makes the buffer
>>validation occur after copying the buffers.
>
> Can these be split into separate patches, please? It's not only always
> good to split logical changes into separate patches but it also makes
> reviewing a lot more pleasant. It's hard to tell from this combined
> patch which changes belong together.

Sure.

>
> I have a few additional comments inline.
>
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>> index f665d67..4f3c004 100644
>> --- a/drivers/gpu/host1x/job.c
>> +++ b/drivers/gpu/host1x/job.c
>> @@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
>> struct host1x_bo *cmdbuf)
>>  void *cmdbuf_page_addr = NULL;
>>
>>  /* pin & patch the relocs for one gather */
>> -while (i < job->num_relocs) {
>> +for (i = 0; i < job->num_relocs; ++i) {
>
> Nit: I prefer post-increment where possible. For consistency.

Will do.

>
>> @@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
>> struct host1x_bo *cmdbuf)
>>  return 0;
>>   }
>>
>> -static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
>> -   unsigned int offset)
>> +static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo 
>> *cmdbuf,
>> +unsigned int offset)
>>   {
>>  offset *= sizeof(u32);
>>
>> -if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
>> -return -EINVAL;
>> +if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
>
> Is the additional !reloc check really necessary? Looking at the callers,
> they always pass in fw->relocarray, which in turn is only NULL if no
> buffers are to be relocated.

Yes, the additional check is necessary exactly for that reason. The code 
will fail if the userspace does not deliver a relocation array and still 
pushes data to an address register.

However, the code *should* check that there are relocations left before 
even coming here so I probably just fix this differently.

>
>> +return true;
>>
>> -return 0;
>> +return false;
>>   }
>
> I wonder whether we should be changing the logic here and have the
> check_reloc() function return true if the relocation is good. I find
> that to be more intuitive.
>

I was also thinking that earlier. Will do.

>> @@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
>> *dev)
>>  int err;
>>  unsigned int i, j;
>>  struct host1x *host = dev_get_drvdata(dev->parent);
>> +
>>  DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));
>
> This is an unnecessary whitespace change.

Ops. Will fix.

- Arto

Re: [PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Arto Merilainen]

On 05/26/2013 01:02 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:

From: Terje Bergstrom tbergst...@nvidia.com

This patch adds several fixes to host1x firewall:
- Host1x firewall does not survive if it expects a reloc, but user
   space didn't pass any relocs. Also it reset the reloc table for
   each gather, whereas they should be reset only per submit. Also
   class does not need to be reset for each class - once per submit
   is enough.
- For INCR opcode the check was not working properly at all.
- The firewall verified gather buffers before copying them. This
   allowed a malicious application to rewrite the buffer content by
   timing the rewrite carefully. This patch makes the buffer
   validation occur after copying the buffers.


Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.


Sure.



I have a few additional comments inline.


diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index f665d67..4f3c004 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
void *cmdbuf_page_addr = NULL;

/* pin  patch the relocs for one gather */
-   while (i  job-num_relocs) {
+   for (i = 0; i  job-num_relocs; ++i) {


Nit: I prefer post-increment where possible. For consistency.


Will do.




@@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
return 0;
  }

-static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
-  unsigned int offset)
+static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
+   unsigned int offset)
  {
offset *= sizeof(u32);

-   if (reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)
-   return -EINVAL;
+   if (!reloc || reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)


Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw-relocarray, which in turn is only NULL if no
buffers are to be relocated.


Yes, the additional check is necessary exactly for that reason. The code 
will fail if the userspace does not deliver a relocation array and still 
pushes data to an address register.


However, the code *should* check that there are relocations left before 
even coming here so I probably just fix this differently.





+   return true;

-   return 0;
+   return false;
  }


I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.



I was also thinking that earlier. Will do.


@@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
*dev)
int err;
unsigned int i, j;
struct host1x *host = dev_get_drvdata(dev-parent);
+
DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));


This is an unnecessary whitespace change.


Ops. Will fix.

- Arto
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Thierry Reding]

On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
> From: Terje Bergstrom 
> 
> This patch adds several fixes to host1x firewall:
> - Host1x firewall does not survive if it expects a reloc, but user
>   space didn't pass any relocs. Also it reset the reloc table for
>   each gather, whereas they should be reset only per submit. Also
>   class does not need to be reset for each class - once per submit
>   is enough.
> - For INCR opcode the check was not working properly at all.
> - The firewall verified gather buffers before copying them. This
>   allowed a malicious application to rewrite the buffer content by
>   timing the rewrite carefully. This patch makes the buffer
>   validation occur after copying the buffers.

Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.

I have a few additional comments inline.

> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index f665d67..4f3c004 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
> struct host1x_bo *cmdbuf)
>   void *cmdbuf_page_addr = NULL;
>  
>   /* pin & patch the relocs for one gather */
> - while (i < job->num_relocs) {
> + for (i = 0; i < job->num_relocs; ++i) {

Nit: I prefer post-increment where possible. For consistency.

> @@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
> struct host1x_bo *cmdbuf)
>   return 0;
>  }
>  
> -static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> -unsigned int offset)
> +static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
> + unsigned int offset)
>  {
>   offset *= sizeof(u32);
>  
> - if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
> - return -EINVAL;
> + if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)

Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw->relocarray, which in turn is only NULL if no
buffers are to be relocated.

> + return true;
>  
> - return 0;
> + return false;
>  }

I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.

> @@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
>   return 0;
>  }
>  
> -static int validate(struct host1x_job *job, struct device *dev,
> - struct host1x_job_gather *g)
> +static int validate_gather(struct host1x_firewall *fw,
> +struct host1x_job_gather *g)

I don't think we necessarily need to rename the function. However since
you modify each line that the rename touches anyway it's okay.

> @@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
> *dev)
>   int err;
>   unsigned int i, j;
>   struct host1x *host = dev_get_drvdata(dev->parent);
> +
>   DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));

This is an unnecessary whitespace change.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

Re: [PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Thierry Reding]

On Fri, May 17, 2013 at 02:49:43PM +0300, Arto Merilainen wrote:
 From: Terje Bergstrom tbergst...@nvidia.com
 
 This patch adds several fixes to host1x firewall:
 - Host1x firewall does not survive if it expects a reloc, but user
   space didn't pass any relocs. Also it reset the reloc table for
   each gather, whereas they should be reset only per submit. Also
   class does not need to be reset for each class - once per submit
   is enough.
 - For INCR opcode the check was not working properly at all.
 - The firewall verified gather buffers before copying them. This
   allowed a malicious application to rewrite the buffer content by
   timing the rewrite carefully. This patch makes the buffer
   validation occur after copying the buffers.

Can these be split into separate patches, please? It's not only always
good to split logical changes into separate patches but it also makes
reviewing a lot more pleasant. It's hard to tell from this combined
patch which changes belong together.

I have a few additional comments inline.

 diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
 index f665d67..4f3c004 100644
 --- a/drivers/gpu/host1x/job.c
 +++ b/drivers/gpu/host1x/job.c
 @@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
 struct host1x_bo *cmdbuf)
   void *cmdbuf_page_addr = NULL;
  
   /* pin  patch the relocs for one gather */
 - while (i  job-num_relocs) {
 + for (i = 0; i  job-num_relocs; ++i) {

Nit: I prefer post-increment where possible. For consistency.

 @@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
 struct host1x_bo *cmdbuf)
   return 0;
  }
  
 -static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 -unsigned int offset)
 +static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 + unsigned int offset)
  {
   offset *= sizeof(u32);
  
 - if (reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)
 - return -EINVAL;
 + if (!reloc || reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)

Is the additional !reloc check really necessary? Looking at the callers,
they always pass in fw-relocarray, which in turn is only NULL if no
buffers are to be relocated.

 + return true;
  
 - return 0;
 + return false;
  }

I wonder whether we should be changing the logic here and have the
check_reloc() function return true if the relocation is good. I find
that to be more intuitive.

 @@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
   return 0;
  }
  
 -static int validate(struct host1x_job *job, struct device *dev,
 - struct host1x_job_gather *g)
 +static int validate_gather(struct host1x_firewall *fw,
 +struct host1x_job_gather *g)

I don't think we necessarily need to rename the function. However since
you modify each line that the rename touches anyway it's okay.

 @@ -508,6 +502,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
 *dev)
   int err;
   unsigned int i, j;
   struct host1x *host = dev_get_drvdata(dev-parent);
 +
   DECLARE_BITMAP(waitchk_mask, host1x_syncpt_nb_pts(host));

This is an unnecessary whitespace change.

Thierry


pgpMd8pehVx5s.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Arto Merilainen]

From: Terje Bergstrom 

This patch adds several fixes to host1x firewall:
- Host1x firewall does not survive if it expects a reloc, but user
  space didn't pass any relocs. Also it reset the reloc table for
  each gather, whereas they should be reset only per submit. Also
  class does not need to be reset for each class - once per submit
  is enough.
- For INCR opcode the check was not working properly at all.
- The firewall verified gather buffers before copying them. This
  allowed a malicious application to rewrite the buffer content by
  timing the rewrite carefully. This patch makes the buffer
  validation occur after copying the buffers.

Signed-off-by: Terje Bergstrom 
Signed-off-by: Arto Merilainen 
---
 drivers/gpu/host1x/job.c |  120 --
 1 file changed, 53 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index f665d67..4f3c004 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
void *cmdbuf_page_addr = NULL;

/* pin & patch the relocs for one gather */
-   while (i < job->num_relocs) {
+   for (i = 0; i < job->num_relocs; ++i) {
struct host1x_reloc *reloc = >relocarray[i];
u32 reloc_addr = (job->reloc_addr_phys[i] +
reloc->target_offset) >> reloc->shift;
u32 *target;

/* skip all other gathers */
-   if (!(reloc->cmdbuf && cmdbuf == reloc->cmdbuf)) {
-   i++;
+   if (cmdbuf != reloc->cmdbuf)
continue;
-   }

if (last_page != reloc->cmdbuf_offset >> PAGE_SHIFT) {
if (cmdbuf_page_addr)
@@ -257,9 +255,6 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)

target = cmdbuf_page_addr + (reloc->cmdbuf_offset & ~PAGE_MASK);
*target = reloc_addr;
-
-   /* mark this gather as handled */
-   reloc->cmdbuf = 0;
}

if (cmdbuf_page_addr)
@@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
return 0;
 }

-static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
-  unsigned int offset)
+static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
+   unsigned int offset)
 {
offset *= sizeof(u32);

-   if (reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
-   return -EINVAL;
+   if (!reloc || reloc->cmdbuf != cmdbuf || reloc->cmdbuf_offset != offset)
+   return true;

-   return 0;
+   return false;
 }

 struct host1x_firewall {
@@ -330,7 +325,7 @@ static int check_incr(struct host1x_firewall *fw)
u32 count = fw->count;
u32 reg = fw->reg;

-   while (fw) {
+   while (count) {
if (fw->words == 0)
return -EINVAL;

@@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
return 0;
 }

-static int validate(struct host1x_job *job, struct device *dev,
-   struct host1x_job_gather *g)
+static int validate_gather(struct host1x_firewall *fw,
+  struct host1x_job_gather *g)
 {
-   u32 *cmdbuf_base;
+   u32 *cmdbuf_base = (u32 *)fw->job->gather_copy_mapped + (g->offset / 4);
int err = 0;
-   struct host1x_firewall fw;

-   fw.job = job;
-   fw.dev = dev;
-   fw.reloc = job->relocarray;
-   fw.num_relocs = job->num_relocs;
-   fw.cmdbuf_id = g->bo;
-
-   fw.offset = 0;
-   fw.class = 0;
-
-   if (!job->is_addr_reg)
+   if (!fw->job->is_addr_reg)
return 0;

-   cmdbuf_base = host1x_bo_mmap(g->bo);
-   if (!cmdbuf_base)
-   return -ENOMEM;
+   fw->words = g->words;
+   fw->cmdbuf_id = g->bo;
+   fw->offset = 0;

-   fw.words = g->words;
-   while (fw.words && !err) {
-   u32 word = cmdbuf_base[fw.offset];
+   while (fw->words && !err) {
+   u32 word = cmdbuf_base[fw->offset];
u32 opcode = (word & 0xf000) >> 28;

-   fw.mask = 0;
-   fw.reg = 0;
-   fw.count = 0;
-   fw.words--;
-   fw.offset++;
+   fw->mask = 0;
+   fw->reg = 0;
+   fw->count = 0;
+   fw->words--;
+   fw->offset++;

switch (opcode) {
case 0:
-   fw.class = word >> 6 & 0x3ff;
-   fw.mask = word & 0x3f;
-   fw.reg = word >> 16 & 0xfff;
-   err = check_mask();
+   fw->class = word 

[PATCH 1/6] gpu: host1x: Fixes to host1x firewall

[Arto Merilainen]

From: Terje Bergstrom tbergst...@nvidia.com

This patch adds several fixes to host1x firewall:
- Host1x firewall does not survive if it expects a reloc, but user
  space didn't pass any relocs. Also it reset the reloc table for
  each gather, whereas they should be reset only per submit. Also
  class does not need to be reset for each class - once per submit
  is enough.
- For INCR opcode the check was not working properly at all.
- The firewall verified gather buffers before copying them. This
  allowed a malicious application to rewrite the buffer content by
  timing the rewrite carefully. This patch makes the buffer
  validation occur after copying the buffers.

Signed-off-by: Terje Bergstrom tbergst...@nvidia.com
Signed-off-by: Arto Merilainen amerilai...@nvidia.com
---
 drivers/gpu/host1x/job.c |  120 --
 1 file changed, 53 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index f665d67..4f3c004 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -228,17 +228,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
void *cmdbuf_page_addr = NULL;
 
/* pin  patch the relocs for one gather */
-   while (i  job-num_relocs) {
+   for (i = 0; i  job-num_relocs; ++i) {
struct host1x_reloc *reloc = job-relocarray[i];
u32 reloc_addr = (job-reloc_addr_phys[i] +
reloc-target_offset)  reloc-shift;
u32 *target;
 
/* skip all other gathers */
-   if (!(reloc-cmdbuf  cmdbuf == reloc-cmdbuf)) {
-   i++;
+   if (cmdbuf != reloc-cmdbuf)
continue;
-   }
 
if (last_page != reloc-cmdbuf_offset  PAGE_SHIFT) {
if (cmdbuf_page_addr)
@@ -257,9 +255,6 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
 
target = cmdbuf_page_addr + (reloc-cmdbuf_offset  ~PAGE_MASK);
*target = reloc_addr;
-
-   /* mark this gather as handled */
-   reloc-cmdbuf = 0;
}
 
if (cmdbuf_page_addr)
@@ -268,15 +263,15 @@ static unsigned int do_relocs(struct host1x_job *job, 
struct host1x_bo *cmdbuf)
return 0;
 }
 
-static int check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
-  unsigned int offset)
+static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
+   unsigned int offset)
 {
offset *= sizeof(u32);
 
-   if (reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)
-   return -EINVAL;
+   if (!reloc || reloc-cmdbuf != cmdbuf || reloc-cmdbuf_offset != offset)
+   return true;
 
-   return 0;
+   return false;
 }
 
 struct host1x_firewall {
@@ -330,7 +325,7 @@ static int check_incr(struct host1x_firewall *fw)
u32 count = fw-count;
u32 reg = fw-reg;
 
-   while (fw) {
+   while (count) {
if (fw-words == 0)
return -EINVAL;
 
@@ -376,69 +371,58 @@ static int check_nonincr(struct host1x_firewall *fw)
return 0;
 }
 
-static int validate(struct host1x_job *job, struct device *dev,
-   struct host1x_job_gather *g)
+static int validate_gather(struct host1x_firewall *fw,
+  struct host1x_job_gather *g)
 {
-   u32 *cmdbuf_base;
+   u32 *cmdbuf_base = (u32 *)fw-job-gather_copy_mapped + (g-offset / 4);
int err = 0;
-   struct host1x_firewall fw;
 
-   fw.job = job;
-   fw.dev = dev;
-   fw.reloc = job-relocarray;
-   fw.num_relocs = job-num_relocs;
-   fw.cmdbuf_id = g-bo;
-
-   fw.offset = 0;
-   fw.class = 0;
-
-   if (!job-is_addr_reg)
+   if (!fw-job-is_addr_reg)
return 0;
 
-   cmdbuf_base = host1x_bo_mmap(g-bo);
-   if (!cmdbuf_base)
-   return -ENOMEM;
+   fw-words = g-words;
+   fw-cmdbuf_id = g-bo;
+   fw-offset = 0;
 
-   fw.words = g-words;
-   while (fw.words  !err) {
-   u32 word = cmdbuf_base[fw.offset];
+   while (fw-words  !err) {
+   u32 word = cmdbuf_base[fw-offset];
u32 opcode = (word  0xf000)  28;
 
-   fw.mask = 0;
-   fw.reg = 0;
-   fw.count = 0;
-   fw.words--;
-   fw.offset++;
+   fw-mask = 0;
+   fw-reg = 0;
+   fw-count = 0;
+   fw-words--;
+   fw-offset++;
 
switch (opcode) {
case 0:
-   fw.class = word  6  0x3ff;
-   fw.mask = word  0x3f;
-   fw.reg = word  16  0xfff;
-   err = check_mask(fw);
+   fw-class = word  6