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

[Mikko Perttunen]

On 14.06.2017 12:06, Dmitry Osipenko wrote:
> 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.

Indeed, we can't fix it, but I was thinking that having the firewall 
enabled in the kernel should always mean that the system is safe, even 
if IOMMU is disabled, and even if it means that you cannot really do 
anything useful with it (at least running host1x_test should be possible 
without shifts, though). Thus no matter what the hardware revision, we 
should reject anything we cannot ensure as safe.

>
>>> +
>>> +    /* 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.

So we have two choices here:
a) If IOMMU is enabled, skip the shift check and return true always. 
This means that we still ensure that the system is safe, since with 
IOMMU enabled it is safe even if the firewall cannot ensure that the job 
is proper. This is what I was trying to say above.
b) Always return false if there is a shift, on any SoC and even if IOMMU 
is enabled. This would harmonize the behavior of the firewall across 
systems.

>
> 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++;
>>>      }
>>>
>
>