| Message ID | 1537539920-30662-4-git-send-email-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86_iommu/amd: add interrupt remap support | expand |
On Fri, Sep 21, 2018 at 02:25:37PM +0000, Singh, Brijesh wrote: > Currently, the amdvi_validate_dte() assumes that a valid DTE will > always have V=1. This is not true. The V=1 means that bit[127:1] are > valid. A valid DTE can have IV=1 and V=0 (i.e address translation > disabled and interrupt remapping enabled) > > Remove the V=1 check from amdvi_validate_dte(), make the caller > responsible to check for V or IV bits. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Cc: Peter Xu <peterx@redhat.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Tom Lendacky <Thomas.Lendacky@amd.com> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > hw/i386/amd_iommu.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c > index 1fd669f..f9aae02 100644 > --- a/hw/i386/amd_iommu.c > +++ b/hw/i386/amd_iommu.c > @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) > AMDVI_DEV_PERM_SHIFT; > } > > -/* a valid entry should have V = 1 and reserved bits honoured */ > +/* validate that reserved bits are honoured */ > static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > uint64_t *dte) > { > @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, > return false; > } > > - return dte[0] & AMDVI_DEV_VALID; > + return true; > } > > /* get a device table entry given the devid */ > @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, > } > > /* devices with V = 0 are not translated */ > - if (!amdvi_get_dte(s, devid, entry)) { > + if (!amdvi_get_dte(s, devid, entry) || > + !(entry[0] & AMDVI_DEV_VALID)) { > goto out; The patch itself looks sane to me, but I noticed that when we do "goto out" we're actually assuming a default passthrough translation. IMHO we should capture the error cases (e.g., non-zero reserved bits) and for those instead of doing translations and DMA we should reject the translation request and report. Otherwise we might have potential risk on guest memory corruption. > } > > -- > 2.7.4 > Regards,
On 9/25/18 1:17 AM, Peter Xu wrote: > On Fri, Sep 21, 2018 at 02:25:37PM +0000, Singh, Brijesh wrote: >> Currently, the amdvi_validate_dte() assumes that a valid DTE will >> always have V=1. This is not true. The V=1 means that bit[127:1] are >> valid. A valid DTE can have IV=1 and V=0 (i.e address translation >> disabled and interrupt remapping enabled) >> >> Remove the V=1 check from amdvi_validate_dte(), make the caller >> responsible to check for V or IV bits. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> Cc: Peter Xu <peterx@redhat.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Richard Henderson <rth@twiddle.net> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> --- >> hw/i386/amd_iommu.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c >> index 1fd669f..f9aae02 100644 >> --- a/hw/i386/amd_iommu.c >> +++ b/hw/i386/amd_iommu.c >> @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) >> AMDVI_DEV_PERM_SHIFT; >> } >> >> -/* a valid entry should have V = 1 and reserved bits honoured */ >> +/* validate that reserved bits are honoured */ >> static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> uint64_t *dte) >> { >> @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, >> return false; >> } >> >> - return dte[0] & AMDVI_DEV_VALID; >> + return true; >> } >> >> /* get a device table entry given the devid */ >> @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, >> } >> >> /* devices with V = 0 are not translated */ >> - if (!amdvi_get_dte(s, devid, entry)) { >> + if (!amdvi_get_dte(s, devid, entry) || >> + !(entry[0] & AMDVI_DEV_VALID)) { >> goto out; > The patch itself looks sane to me, but I noticed that when we do "goto > out" we're actually assuming a default passthrough translation. IMHO > we should capture the error cases (e.g., non-zero reserved bits) and > for those instead of doing translations and DMA we should reject the > translation request and report. Otherwise we might have potential > risk on guest memory corruption. > OK, I can break check as below and log the error if (!amdvi_get_dte(s, devid, entry)) { /* log error */ } if (!(entry[0] & AMDVI_DEV_VALID)) { goto out; /* pass through */ } >> >> -- >> 2.7.4 >> > Regards, >
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c index 1fd669f..f9aae02 100644 --- a/hw/i386/amd_iommu.c +++ b/hw/i386/amd_iommu.c @@ -807,7 +807,7 @@ static inline uint64_t amdvi_get_perms(uint64_t entry) AMDVI_DEV_PERM_SHIFT; } -/* a valid entry should have V = 1 and reserved bits honoured */ +/* validate that reserved bits are honoured */ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, uint64_t *dte) { @@ -820,7 +820,7 @@ static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid, return false; } - return dte[0] & AMDVI_DEV_VALID; + return true; } /* get a device table entry given the devid */ @@ -967,7 +967,8 @@ static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr, } /* devices with V = 0 are not translated */ - if (!amdvi_get_dte(s, devid, entry)) { + if (!amdvi_get_dte(s, devid, entry) || + !(entry[0] & AMDVI_DEV_VALID)) { goto out; }
Currently, the amdvi_validate_dte() assumes that a valid DTE will always have V=1. This is not true. The V=1 means that bit[127:1] are valid. A valid DTE can have IV=1 and V=0 (i.e address translation disabled and interrupt remapping enabled) Remove the V=1 check from amdvi_validate_dte(), make the caller responsible to check for V or IV bits. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> Cc: Peter Xu <peterx@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <rth@twiddle.net> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: Tom Lendacky <Thomas.Lendacky@amd.com> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> --- hw/i386/amd_iommu.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)