Re: [PATCH] iommu/vt-d: debugfs: Check irq_remapping_cap before PI info dump

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2021/1/20 16:41, Zhang, Tina wrote:
> 
> 
>> -----Original Message-----
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, January 20, 2021 10:35 AM
>> To: Zhang, Tina <tina.zhang@intel.com>
>> Cc: baolu.lu@linux.intel.com; iommu@lists.linux-foundation.org; Joerg
>> Roedel <joro@8bytes.org>; Mehta, Sohil <sohil.mehta@intel.com>; Jacob
>> Pan <jacob.jun.pan@linux.intel.com>; Sun, Yi <yi.sun@intel.com>
>> Subject: Re: [PATCH] iommu/vt-d: debugfs: Check irq_remapping_cap before
>> PI info dump
>>
>> On 1/20/21 2:25 AM, Tina Zhang wrote:
>>> irq_remapping_cap() was introduced to detect whether irq remapping
>>> supports new features, such as VT-d Posted-Interrupts", according to
>>> commit 959c870f7305 ("iommu, x86: Provide irq_remapping_cap()
>> interface").
>>>
>>> The VT-d Posted-Interrupts support can be disabled by the command line
>>> parameter "intremap=nopost".
>>>
>>> So, it's better to use irq_remapping_cap() to check if the VT-d
>>> Posted-Interrupts is enabled before any Posted Interrupt Descriptor
>>> info dump.
>>>
>>> Cc: Lu Baolu <baolu.lu@linux.intel.com>
>>> Cc: Joerg Roedel <joro@8bytes.org>
>>> Cc: Sohil Mehta <sohil.mehta@intel.com>
>>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>> Reported-by: Yi Sun <yi.sun@intel.com>
>>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>>> ---
>>>    drivers/iommu/intel/debugfs.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/intel/debugfs.c
>>> b/drivers/iommu/intel/debugfs.c index efea7f02abd9..87a4a76866f4
>>> 100644
>>> --- a/drivers/iommu/intel/debugfs.c
>>> +++ b/drivers/iommu/intel/debugfs.c
>>> @@ -516,7 +516,8 @@ static int ir_translation_struct_show(struct seq_file
>> *m, void *unused)
>>>    	seq_puts(m, "****\n\n");
>>>
>>>    	for_each_active_iommu(iommu, drhd) {
>>> -		if (!cap_pi_support(iommu->cap))
>>> +		if (!irq_remapping_cap(IRQ_POSTING_CAP) ||
>>> +		    !cap_pi_support(iommu->cap))
>>
>> With irq_remapping_cap(IRQ_POSTING_CAP), do you still need
>> cap_pi_support(iommu->cap)?
> 
> I guess yes. The "iommu->cap" value comes from the iommu reg. Current code seems to use cap_pi_suport() to check if the iommu hardware supports PI capability, meanwhile using irq_remapping_cap() to see if the vt-d PI support is enabled by user.
> 
> So, the problem here is if a user explicitly disables the vt-d PI support by "intremap=nopost", it would be very confused that the PI descriptor related info can still get dump.

I don't worry about dump hardware data even it's not enabled. But I do
care that the table is not allocated (due to not enabled) but the code
still tries to dump it, hence result in some kinds of NULL pointer or
wild pointer referencing.

Best regards,
baolu

> 
> Thanks,
> Tina
> 
>>
>>>    			continue;
>>>
>>>    		seq_printf(m, "Posted Interrupt supported on
>> IOMMU: %s\n",
>>>
>>
>> Best regards,
>> baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu