From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/1] amd/iommu: Fix infinite loop when handling IO_PAGE_FAULT event Date: Sun, 29 Dec 2013 16:33:04 +0000 Message-ID: <52C04EC0.4010406@citrix.com> References: <1388309750-4495-1-git-send-email-suravee.suthikulpanit@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1388309750-4495-1-git-send-email-suravee.suthikulpanit@amd.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: suravee.suthikulpanit@amd.com, JBeulich@suse.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 29/12/2013 09:35, suravee.suthikulpanit@amd.com wrote: > From: Suravee Suthikulpanit > > 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 > --- > 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[] = {