[PATCH 1/1] amd/iommu: Fix infinite loop when handling IO_PAGE_FAULT event

[PATCH 1/1] amd/iommu: Fix infinite loop when handling IO_PAGE_FAULT event

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Certain AMD systems could have upto 0x1000 ivrs_bdf_entries.
However, the loop variable (bdf) is declared as u16 which causes
inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
This patch changes the variable to u32 instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
NOTE: I found this issue on both stable-4.3 and master branches.
Do you think we can also back port this change to 4.3 as well?

 xen/drivers/passthrough/amd/iommu_init.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index b431d16..b96a4af 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -524,8 +524,8 @@ static hw_irq_controller iommu_maskable_msi_type = {
 
 static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
 {
-    u16 domain_id, device_id, bdf, flags;
-    u32 code;
+    u16 domain_id, device_id, flags;
+    u32 code, bdf;
     u64 *addr;
     int count = 0;
     static const char *const event_str[] = {
-- 
1.7.10.4

Re: [PATCH 1/1] amd/iommu: Fix infinite loop when handling IO_PAGE_FAULT event

[Andrew Cooper]

On 29/12/2013 09:35, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> Certain AMD systems could have upto 0x1000 ivrs_bdf_entries.
> However, the loop variable (bdf) is declared as u16 which causes
> inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
> This patch changes the variable to u32 instead.

Do you perhaps mean that there could be 0x10000 ivrs_bdf_entries? 
Otherwise I cant see how an infinite loop is possible.

On the other hand, assuming that the infinite loop is possible, it is
also vulnerable in register_exclusion_range_for_{all,iommu}_devices(),
which also have similar for loops with a u16 bdf.

Even if you do promote to a u32, the get_dma_requestor_id() call now
truncates a u32 to a u16, so can now return the wrong device.

Beyond that, there is already quite a mix of u32, int and u16's for
various bdf values across the this area of the code, with plenty of
truncation issues at a glance.

~Andrew

>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> NOTE: I found this issue on both stable-4.3 and master branches.
> Do you think we can also back port this change to 4.3 as well?
>
>  xen/drivers/passthrough/amd/iommu_init.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index b431d16..b96a4af 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -524,8 +524,8 @@ static hw_irq_controller iommu_maskable_msi_type = {
>  
>  static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
>  {
> -    u16 domain_id, device_id, bdf, flags;
> -    u32 code;
> +    u16 domain_id, device_id, flags;
> +    u32 code, bdf;
>      u64 *addr;
>      int count = 0;
>      static const char *const event_str[] = {

Re: [PATCH 1/1] amd/iommu: Fix infinite loop when handling IO_PAGE_FAULT event

[Suravee Suthikulpanit]

On 12/29/2013 11:33 PM, Andrew Cooper wrote:
> On 29/12/2013 09:35, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Certain AMD systems could have upto 0x1000 ivrs_bdf_entries.
>> However, the loop variable (bdf) is declared as u16 which causes
>> inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
>> This patch changes the variable to u32 instead.
>
> Do you perhaps mean that there could be 0x10000 ivrs_bdf_entries?
> Otherwise I cant see how an infinite loop is possible.

Ah Yes, This is actually 0x10000. Sorry for the typo.

> On the other hand, assuming that the infinite loop is possible, it is
> also vulnerable in register_exclusion_range_for_{all,iommu}_devices(),
> which also have similar for loops with a u16 bdf.

Thanks for catching the rest here.  I'll clean them up also and send out V2.

> Even if you do promote to a u32, the get_dma_requestor_id() call now
> truncates a u32 to a u16, so can now return the wrong device.

Actually, bdf should only be 16 bits. However, I think we just need to 
resolve the looping logic.  The truncation should not cause issue here.

>
> Beyond that, there is already quite a mix of u32, int and u16's for
> various bdf values across the this area of the code, with plenty of
> truncation issues at a glance.
>
> ~Andrew
>
I'll try to go through them and clean up in V2.

Suravee