Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

2018-05-11 Thread Goel, Sameer 


On 4/12/2018 5:56 AM, Marc Zyngier wrote:
> On 12/04/18 11:17, Robin Murphy wrote:
>> On 11/04/18 17:54, Marc Zyngier wrote:
>>> Hi Sammer,
>>>
>>> On 11/04/18 16:58, Goel, Sameer wrote:
>>>>
>>>>
>>>> On 3/28/2018 9:00 AM, Marc Zyngier wrote:
>>>>> On 2018-03-28 15:39, Timur Tabi wrote:
>>>>>> From: Sameer Goel <sg...@codeaurora.org>
>>>>>>
>>>>>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>>>>>> when SMMUEN==0.
>>>>>>
>>>>>> This prevents a race condition where a stray DMA from the crashed primary
>>>>>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>>>>>> disabled during reset in the crash kernel.
>>>>>>
>>>>>> Signed-off-by: Sameer Goel <sg...@codeaurora.org>
>>>>>> ---
>>>>>>   drivers/iommu/arm-smmu-v3.c | 12 
>>>>>>   1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>>>>> index 3f2f1fc68b52..c04a89310c59 100644
>>>>>> --- a/drivers/iommu/arm-smmu-v3.c
>>>>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>>>>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct
>>>>>> arm_smmu_device *smmu, bool bypass)
>>>>>>   if (reg & CR0_SMMUEN)
>>>>>>   dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>>>>>
>>>>>> +    /*
>>>>>> + * Abort all incoming translations. This can happen in a kdump case
>>>>>> + * where SMMU is initialized when a prior DMA is pending. Just
>>>>>> + * disabling the SMMU in this case might result in writes to invalid
>>>>>> + * PAs.
>>>>>> + */
>>>>>> +    ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>>>>>> +    if (ret) {
>>>>>> +    dev_err(smmu->dev, "GBPA not responding to update\n");
>>>>>> +    return ret;
>>>>>> +    }
>>>>>> +
>>>>>>   ret = arm_smmu_device_disable(smmu);
>>>>>>   if (ret)
>>>>>>   return ret;
>>>>>
>>>>> A tangential question: can we reliably detect that the SMMU already
>>>>> has valid mappings, which would indicate that we're in a pretty bad
>>>>> shape already by the time we set that bit? For all we know, memory
>>>>> could have been corrupted long before we hit this point, and this
>>>>> patch barely narrows the window of opportunity.
>>>>
>>>> :) Yes that is correct. This only covers the kdump scenario. Trying
>>>> to get some reliability when booting up the crash kernel. The system
>>>> is already in a bad state. I don't think that this will happen in a
>>>> normal scenario. But please point me to the GICv3 change and I'll
>>>> have a look.
>>>
>>> See this:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/tree/drivers/irqchip/irq-gic-v3-its.c?h=irq/irqchip-4.17=6eb486b66a3094cdcd68dc39c9df3a29d6a51dd5#n3407
>>
>> The nearest equivalent to that is probably the top-level SMMUEN check 
>> that we already have (see the diff context above). To go beyond that 
>> you'd have to chase the old stream table pointer and scan the whole 
>> thing looking for valid contexts, then potentially walk page tables 
>> within those contexts to check for live translations if you really 
>> wanted to be sure. That would be a hell of a lot of work to do in the 
>> boot path.
> Yeah, feels a bit too involved for sanity. I'd simply suggest you taint
> the kernel if you find the SMMU enabled, as you're already on shaky ground.

Ok. I think since this is a kdump kernel a taint is not necessary?
> 
> Thanks,
> 
>   M.
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

2018-04-11 Thread Goel, Sameer 


On 3/28/2018 9:00 AM, Marc Zyngier wrote:
> On 2018-03-28 15:39, Timur Tabi wrote:
>> From: Sameer Goel 
>>
>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>> when SMMUEN==0.
>>
>> This prevents a race condition where a stray DMA from the crashed primary
>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>> disabled during reset in the crash kernel.
>>
>> Signed-off-by: Sameer Goel 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 3f2f1fc68b52..c04a89310c59 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct
>> arm_smmu_device *smmu, bool bypass)
>>  if (reg & CR0_SMMUEN)
>>  dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>
>> +    /*
>> + * Abort all incoming translations. This can happen in a kdump case
>> + * where SMMU is initialized when a prior DMA is pending. Just
>> + * disabling the SMMU in this case might result in writes to invalid
>> + * PAs.
>> + */
>> +    ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>> +    if (ret) {
>> +    dev_err(smmu->dev, "GBPA not responding to update\n");
>> +    return ret;
>> +    }
>> +
>>  ret = arm_smmu_device_disable(smmu);
>>  if (ret)
>>  return ret;
> 
> A tangential question: can we reliably detect that the SMMU already has valid 
> mappings, which would indicate that we're in a pretty bad shape already by 
> the time we set that bit? For all we know, memory could have been corrupted 
> long before we hit this point, and this patch barely narrows the window of 
> opportunity.
:) Yes that is correct. This only covers the kdump scenario. Trying to get some 
reliability when booting up the crash kernel. The system is already in a bad 
state. I don't think that this will happen in a normal scenario. But please 
point me to the GICv3 change and I'll have a look.
Thanks,
Sameer 
> 
> At the very least, we should emit a warning and taint the kernel (we've 
> recently added such measures to the GICv3 driver).
> 
> Thanks,
> 
>     M.

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/arm-smmu-v3: Set GBPA to abort all transactions

2018-04-11 Thread Goel, Sameer 


On 4/5/2018 5:26 AM, Will Deacon wrote:
> On Wed, Mar 28, 2018 at 09:39:40AM -0500, Timur Tabi wrote:
>> From: Sameer Goel 
>>
>> Set SMMU_GBPA to abort all incoming translations during the SMMU reset
>> when SMMUEN==0.
>>
>> This prevents a race condition where a stray DMA from the crashed primary
>> kernel can try to access an IOVA address as an invalid PA when SMMU is
>> disabled during reset in the crash kernel.
>>
>> Signed-off-by: Sameer Goel 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 3f2f1fc68b52..c04a89310c59 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2458,6 +2458,18 @@ static int arm_smmu_device_reset(struct 
>> arm_smmu_device *smmu, bool bypass)
>>  if (reg & CR0_SMMUEN)
>>  dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
>>  
>> +/*
>> + * Abort all incoming translations. This can happen in a kdump case
>> + * where SMMU is initialized when a prior DMA is pending. Just
>> + * disabling the SMMU in this case might result in writes to invalid
>> + * PAs.
>> + */
>> +ret = arm_smmu_update_gbpa(smmu, 1, GBPA_ABORT);
>> +if (ret) {
>> +dev_err(smmu->dev, "GBPA not responding to update\n");
>> +return ret;
>> +}
> 
> This needs to be predicated on the disable_bypass option, otherwise I think
> it will cause regressions for systems that rely on passthrough.
Ok, I'll make the change.
> 
> Will
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu