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

[Mikko Perttunen]

On 06/14/2017 05:49 PM, Dmitry Osipenko wrote:
> On 14.06.2017 14:47, Mikko Perttunen wrote:
>>
>>
>> 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.
> 
> Okay, I think b) is a better choice, actually that's what V1 of the patch did.
> Preserving behaviour regardless of the IOMMU state is reasonable and this is an
> opensource driver that is supposed to be used with the opensource userspace
> after all.
> 
> Later I'd want to stricter the firewall checks to disallow any unknown register
> accesses, that will make it a bit more secure on a later SoC's as well. After,
> we should add a memory access bounds checking for each of HW operations to make
> it really secure.
> 

Yep, this sounds good.

Mikko