linux-kernel - Re: [PATCH] iommu/amd: fix interrupt remapping for avic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Sep 2020 14:25:01 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        linux-kernel@...r.kernel.org
Cc:     "open list:AMD IOMMU (AMD-VI)" <iommu@...ts.linux-foundation.org>,
        Joao Martins <joao.m.martins@...cle.com>,
        Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH] iommu/amd: fix interrupt remapping for avic

On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote:
> Maxim,
> 
> On 9/13/2020 7:42 PM, Maxim Levitsky wrote:
> > Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE")
> > accidentally removed an assumption that modify_irte_ga always set the valid bit
> > and amd_iommu_activate_guest_mode relied on that.
> > 
> > Side effect of this is that on my machine, VFIO based VMs with AVIC enabled
> > would eventually crash and show IOMMU errors like that:
> > 
> > AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008]
> > 
> > Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE")
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> >   drivers/iommu/amd/iommu.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index 07ae8b93887e5..aff4cc1869356 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data)
> >   	entry->hi.fields.ga_root_ptr       = ir_data->ga_root_ptr;
> >   	entry->hi.fields.vector            = ir_data->ga_vector;
> >   	entry->lo.fields_vapic.ga_tag      = ir_data->ga_tag;
> > +	entry->lo.fields_remap.valid = 1;
> >   
> >   	return modify_irte_ga(ir_data->irq_2_irte.devid,
> >   			      ir_data->irq_2_irte.index, entry, ir_data);
> > 
> 
> Could you please try with the following patch instead?
> 
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data)
>   {
>          struct amd_ir_data *ir_data = (struct amd_ir_data *)data;
>          struct irte_ga *entry = (struct irte_ga *) ir_data->entry;
> +       u64 valid;
> 
>          if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) ||
>              !entry || entry->lo.fields_vapic.guest_mode)
>                  return 0;
> 
> +       valid = entry->lo.fields_vapic.valid;
> +
>          entry->lo.val = 0;
>          entry->hi.val = 0;
> 
> +       entry->lo.fields_vapic.valid       = valid;
>          entry->lo.fields_vapic.guest_mode  = 1;
>          entry->lo.fields_vapic.ga_log_intr = 1;
>          entry->hi.fields.ga_root_ptr       = ir_data->ga_root_ptr;
> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data)
>          struct amd_ir_data *ir_data = (struct amd_ir_data *)data;
>          struct irte_ga *entry = (struct irte_ga *) ir_data->entry;
>          struct irq_cfg *cfg = ir_data->cfg;
> -       u64 valid = entry->lo.fields_remap.valid;
> +       u64 valid;
> 
>          if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) ||
>              !entry || !entry->lo.fields_vapic.guest_mode)
>                  return 0;
> 
> +       valid = entry->lo.fields_remap.valid;
> +
>          entry->lo.val = 0;
>          entry->hi.val = 0;
I see. I based my approach on the fact that valid bit was
set always to true anyway before, plus that amd_iommu_activate_guest_mode
should be really only called when someone activates a valid interrupt remapping
entry, but IMHO the approach of preserving the valid bit is safer anyway.

It works on my system (I applied the patch manually, since either your or my email client,
seems to mangle the patch)

Thanks,
Best regards,
	Maxim Levitsky


> --

> 
> Thanks,
> Suravee
>