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 <[email protected]> > Cc: Peter Xu <[email protected]> > Cc: "Michael S. Tsirkin" <[email protected]> > Cc: Paolo Bonzini <[email protected]> > Cc: Richard Henderson <[email protected]> > Cc: Eduardo Habkost <[email protected]> > Cc: Marcel Apfelbaum <[email protected]> > Cc: Tom Lendacky <[email protected]> > Cc: Suravee Suthikulpanit <[email protected]> > --- > 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, -- Peter Xu
