[PATCH v2 14/22] gpu: host1x: Forbid relocation address shifting in the firewall

[Dmitry Osipenko]

On 14.06.2017 09:50, Mikko Perttunen wrote:
> On 14.06.2017 02:15, Dmitry Osipenko wrote:
>> 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 <digetx at gmail.com>
>> Reviewed-by: Erik Faye-Lund <kusmabite at gmail.com>
>> ---
>>  drivers/gpu/host1x/dev.c |  4 ++++
>>  drivers/gpu/host1x/dev.h |  1 +
>>  drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
>> index 6a805ed908c3..21da331ce092 100644
>> --- a/drivers/gpu/host1x/dev.c
>> +++ b/drivers/gpu/host1x/dev.c
>> @@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
>>      .init = host1x01_init,
>>      .sync_offset = 0x3000,
>>      .dma_mask = DMA_BIT_MASK(32),
>> +    .version = 1,
>>  };
>>
>>  static const struct host1x_info host1x02_info = {
>> @@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
>>      .init = host1x02_init,
>>      .sync_offset = 0x3000,
>>      .dma_mask = DMA_BIT_MASK(32),
>> +    .version = 2,
>>  };
>>
>>  static const struct host1x_info host1x04_info = {
>> @@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
>>      .init = host1x04_init,
>>      .sync_offset = 0x2100,
>>      .dma_mask = DMA_BIT_MASK(34),
>> +    .version = 4,
>>  };
>>
>>  static const struct host1x_info host1x05_info = {
>> @@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
>>      .init = host1x05_init,
>>      .sync_offset = 0x2100,
>>      .dma_mask = DMA_BIT_MASK(34),
>> +    .version = 5,
>>  };
>>
>>  static const struct of_device_id host1x_of_match[] = {
>> diff --git a/drivers/gpu/host1x/dev.h b/drivers/gpu/host1x/dev.h
>> index 229d08b6a45e..c6997651b336 100644
>> --- a/drivers/gpu/host1x/dev.h
>> +++ b/drivers/gpu/host1x/dev.h
>> @@ -100,6 +100,7 @@ struct host1x_info {
>>      int (*init)(struct host1x *host1x); /* initialize per SoC ops */
>>      unsigned int sync_offset; /* offset of syncpoint registers */
>>      u64 dma_mask; /* mask of addressable memory */
>> +    unsigned int version; /* host1x's version */
>>  };
>>
>>  struct host1x {
>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>> index 4208329ca2af..7825643da324 100644
>> --- a/drivers/gpu/host1x/job.c
>> +++ b/drivers/gpu/host1x/job.c
>> @@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc,
>> struct host1x_bo *cmdbuf,
>>      return true;
>>  }
>>
>> +static bool check_reloc_shift(struct device *dev, struct host1x_reloc *reloc)
>> +{
>> +    struct host1x *host = dev_get_drvdata(dev->parent);
>> +
>> +    /* skip newer Tegra's since IOMMU is supposed to be used by
>> +     * them and not all address registers with their shifts are
>> +     * publicly documented */
>> +    if (host->info->version > 1)
>> +        return true;
> 
> I don't like the firewall working differently per SoC - IOMMU can still be
> disabled on later SoCs and in this case there would be a security hole even if
> the firewall is enabled.
> 

Yes, it would be a security hole like it is is right now. Unfortunately we can't
fix it on later SoC's without knowing all the address registers and their shifts.

>> +
>> +    /* skip Tegra30 with IOMMU enabled */
>> +    if (host->domain)
>> +        return true;
>> +
> 
> So if we want to be able to test the firewall on all SoCs just having this check
> here for all SoCs would be better.
> 

I'm not sure what you are meaning here. You'll get a wrong address resolved in
HW if that address needs to be shifted.

As I wrote before, on later SoC's firewall could be used as a debug feature
without the shifts being validated.

>> +    /* relocation shift value validation isn't implemented yet */
>> +    if (reloc->shift)
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>>  struct host1x_firewall {
>>      struct host1x_job *job;
>>      struct device *dev;
>> @@ -359,6 +380,9 @@ static int check_register(struct host1x_firewall *fw,
>> unsigned long offset)
>>          if (!check_reloc(fw->reloc, fw->cmdbuf, fw->offset))
>>              return -EINVAL;
>>
>> +        if (!check_reloc_shift(fw->dev, fw->reloc))
>> +            return -EINVAL;
>> +
>>          fw->num_relocs--;
>>          fw->reloc++;
>>      }
>>


-- 
Dmitry