| Message ID | 1506518920-18571-4-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 27/09/17 14:28, Eric Auger wrote: > At the moment the device table save() returns -EINVAL if > vgic_its_check_id() fails to return the gpa of the entry > associated to the device/collection id. Let vgic_its_check_id() > return an int instead of a bool and return a more precised > error value: > - EINVAL in case the id is out of range > - EFAULT if the gpa is not provisionned or is not valid > > We also check first the GITS_BASER<n> Valid bit is set. > > This allows the userspace to discriminate failure reasons. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > need to CC stable > --- > virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 36 insertions(+), 17 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 76bed2d..c1f7972 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its, > } > > /* kernel-doc requires two asterisks to recognise conforming comments [1]. > - * Check whether an ID can be stored into the corresponding guest table. > + * vgic_its_check_id - Check whether an ID can be stored into > + * the corresponding guest table. > + * > * For a direct table this is pretty easy, but gets a bit nasty for > * indirect tables. We check whether the resulting guest physical address > * is actually valid (covered by a memslot and guest accessible). > * For this we have to read the respective first level entry. And the more elaborate explanations should come after the arguments section. The rest looks OK. Cheers, Andre. [1] Documentation/kernel-doc-nano-HOWTO.txt > + * > + * @its: its handle > + * @baser: GITS_BASER<n> register > + * @id: id of the device/collection > + * @eaddr: output gpa of the corresponding table entry > + * > + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if > + * the address cannot be computed or is not valid > */ > -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > - gpa_t *eaddr) > +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > + gpa_t *eaddr) > { > int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; > u64 indirect_ptr, type = GITS_BASER_TYPE(baser); > @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > int index; > gfn_t gfn; > > + if (!(baser & GITS_BASER_VALID)) > + return -EFAULT; > + > switch (type) { > case GITS_BASER_TYPE_DEVICE: > if (id >= BIT_ULL(VITS_TYPER_DEVBITS)) > - return false; > + return -EINVAL; > break; > case GITS_BASER_TYPE_COLLECTION: > /* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */ > if (id >= BIT_ULL(16)) > - return false; > + return -EINVAL; > break; > default: > - return false; > + return -EINVAL; > } > > if (!(baser & GITS_BASER_INDIRECT)) { > phys_addr_t addr; > > if (id >= (l1_tbl_size / esz)) > - return false; > + return -EINVAL; > > addr = BASER_ADDRESS(baser) + id * esz; > gfn = addr >> PAGE_SHIFT; > > if (eaddr) > *eaddr = addr; > - return kvm_is_visible_gfn(its->dev->kvm, gfn); > + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) > + return 0; > + else > + return -EFAULT; > } > > /* calculate and check the index into the 1st level */ > index = id / (SZ_64K / esz); > if (index >= (l1_tbl_size / sizeof(u64))) > - return false; > + return -EINVAL; > > /* Each 1st level entry is represented by a 64-bit value. */ > if (kvm_read_guest(its->dev->kvm, > BASER_ADDRESS(baser) + index * sizeof(indirect_ptr), > &indirect_ptr, sizeof(indirect_ptr))) > - return false; > + return -EFAULT; > > indirect_ptr = le64_to_cpu(indirect_ptr); > > /* check the valid bit of the first level entry */ > if (!(indirect_ptr & BIT_ULL(63))) > - return false; > + return -EFAULT; > > /* > * Mask the guest physical address and calculate the frame number. > @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > > if (eaddr) > *eaddr = indirect_ptr; > - return kvm_is_visible_gfn(its->dev->kvm, gfn); > + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) > + return 0; > + else > + return -EFAULT; > } > > static int vgic_its_alloc_collection(struct vgic_its *its, > @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its, > { > struct its_collection *collection; > > - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) > + if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) > return E_ITS_MAPC_COLLECTION_OOR; > > collection = kzalloc(sizeof(*collection), GFP_KERNEL); > @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, > gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); > struct its_device *device; > > - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) > + if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) > return E_ITS_MAPD_DEVICE_OOR; > > if (valid && num_eventid_bits > VITS_TYPER_IDBITS) > @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its) > int ret; > gpa_t eaddr; > > - if (!vgic_its_check_id(its, baser, > - dev->device_id, &eaddr)) > - return -EINVAL; > + ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr); > + if (ret) > + return ret; > > ret = vgic_its_save_itt(its, dev); > if (ret) >
On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote: > At the moment the device table save() returns -EINVAL if > vgic_its_check_id() fails to return the gpa of the entry > associated to the device/collection id. Let vgic_its_check_id() > return an int instead of a bool and return a more precised > error value: > - EINVAL in case the id is out of range > - EFAULT if the gpa is not provisionned or is not valid > This is just to ease debugging, yes? Seems a bit invasive to me for that purpose. > We also check first the GITS_BASER<n> Valid bit is set. > > This allows the userspace to discriminate failure reasons. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > need to CC stable Is this solving a real issue that someone has? Otherwise I don't think it should be cc'ed to stable, nor am I sure it warrants merging as a fix. Thanks, -Christoffer > --- > virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 36 insertions(+), 17 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 76bed2d..c1f7972 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its, > } > > /* > - * Check whether an ID can be stored into the corresponding guest table. > + * vgic_its_check_id - Check whether an ID can be stored into > + * the corresponding guest table. > + * > * For a direct table this is pretty easy, but gets a bit nasty for > * indirect tables. We check whether the resulting guest physical address > * is actually valid (covered by a memslot and guest accessible). > * For this we have to read the respective first level entry. > + * > + * @its: its handle > + * @baser: GITS_BASER<n> register > + * @id: id of the device/collection > + * @eaddr: output gpa of the corresponding table entry > + * > + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if > + * the address cannot be computed or is not valid > */ > -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > - gpa_t *eaddr) > +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > + gpa_t *eaddr) > { > int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; > u64 indirect_ptr, type = GITS_BASER_TYPE(baser); > @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > int index; > gfn_t gfn; > > + if (!(baser & GITS_BASER_VALID)) > + return -EFAULT; > + > switch (type) { > case GITS_BASER_TYPE_DEVICE: > if (id >= BIT_ULL(VITS_TYPER_DEVBITS)) > - return false; > + return -EINVAL; > break; > case GITS_BASER_TYPE_COLLECTION: > /* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */ > if (id >= BIT_ULL(16)) > - return false; > + return -EINVAL; > break; > default: > - return false; > + return -EINVAL; > } > > if (!(baser & GITS_BASER_INDIRECT)) { > phys_addr_t addr; > > if (id >= (l1_tbl_size / esz)) > - return false; > + return -EINVAL; > > addr = BASER_ADDRESS(baser) + id * esz; > gfn = addr >> PAGE_SHIFT; > > if (eaddr) > *eaddr = addr; > - return kvm_is_visible_gfn(its->dev->kvm, gfn); > + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) > + return 0; > + else > + return -EFAULT; > } > > /* calculate and check the index into the 1st level */ > index = id / (SZ_64K / esz); > if (index >= (l1_tbl_size / sizeof(u64))) > - return false; > + return -EINVAL; > > /* Each 1st level entry is represented by a 64-bit value. */ > if (kvm_read_guest(its->dev->kvm, > BASER_ADDRESS(baser) + index * sizeof(indirect_ptr), > &indirect_ptr, sizeof(indirect_ptr))) > - return false; > + return -EFAULT; > > indirect_ptr = le64_to_cpu(indirect_ptr); > > /* check the valid bit of the first level entry */ > if (!(indirect_ptr & BIT_ULL(63))) > - return false; > + return -EFAULT; > > /* > * Mask the guest physical address and calculate the frame number. > @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, > > if (eaddr) > *eaddr = indirect_ptr; > - return kvm_is_visible_gfn(its->dev->kvm, gfn); > + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) > + return 0; > + else > + return -EFAULT; > } > > static int vgic_its_alloc_collection(struct vgic_its *its, > @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its, > { > struct its_collection *collection; > > - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) > + if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) > return E_ITS_MAPC_COLLECTION_OOR; > > collection = kzalloc(sizeof(*collection), GFP_KERNEL); > @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, > gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); > struct its_device *device; > > - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) > + if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) > return E_ITS_MAPD_DEVICE_OOR; > > if (valid && num_eventid_bits > VITS_TYPER_IDBITS) > @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its) > int ret; > gpa_t eaddr; > > - if (!vgic_its_check_id(its, baser, > - dev->device_id, &eaddr)) > - return -EINVAL; > + ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr); > + if (ret) > + return ret; > > ret = vgic_its_save_itt(its, dev); > if (ret) > -- > 2.5.5 >
Hi, On 13/10/2017 15:16, Christoffer Dall wrote: > On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote: >> At the moment the device table save() returns -EINVAL if >> vgic_its_check_id() fails to return the gpa of the entry >> associated to the device/collection id. Let vgic_its_check_id() >> return an int instead of a bool and return a more precised >> error value: >> - EINVAL in case the id is out of range >> - EFAULT if the gpa is not provisionned or is not valid >> > > This is just to ease debugging, yes? I understood user-space should be able to discriminate between bad guest programming and values corrupted by the userspace (regs for instance). In first case QEMU should not abort. In latter case it should abort. In vgic_its_check_id we are checking the L1 entry validity bit and in case it is invalid we can't compute the GPA of the entry. I was thinking we should return -EFAULT in that case. But maybe returning EFAULT in case the BASER<n> address is not reachable also is wrong because that may be caused by the userspace writing a wrong value. Sigh ... Thanks Eric > > Seems a bit invasive to me for that purpose. > >> We also check first the GITS_BASER<n> Valid bit is set. >> >> This allows the userspace to discriminate failure reasons. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> need to CC stable > > Is this solving a real issue that someone has? Otherwise I don't think > it should be cc'ed to stable, nor am I sure it warrants merging as a > fix. > > Thanks, > -Christoffer > >> --- >> virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++-------------- >> 1 file changed, 36 insertions(+), 17 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 76bed2d..c1f7972 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its, >> } >> >> /* >> - * Check whether an ID can be stored into the corresponding guest table. >> + * vgic_its_check_id - Check whether an ID can be stored into >> + * the corresponding guest table. >> + * >> * For a direct table this is pretty easy, but gets a bit nasty for >> * indirect tables. We check whether the resulting guest physical address >> * is actually valid (covered by a memslot and guest accessible). >> * For this we have to read the respective first level entry. >> + * >> + * @its: its handle >> + * @baser: GITS_BASER<n> register >> + * @id: id of the device/collection >> + * @eaddr: output gpa of the corresponding table entry >> + * >> + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if >> + * the address cannot be computed or is not valid >> */ >> -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, >> - gpa_t *eaddr) >> +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, >> + gpa_t *eaddr) >> { >> int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; >> u64 indirect_ptr, type = GITS_BASER_TYPE(baser); >> @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, >> int index; >> gfn_t gfn; >> >> + if (!(baser & GITS_BASER_VALID)) >> + return -EFAULT; >> + >> switch (type) { >> case GITS_BASER_TYPE_DEVICE: >> if (id >= BIT_ULL(VITS_TYPER_DEVBITS)) >> - return false; >> + return -EINVAL; >> break; >> case GITS_BASER_TYPE_COLLECTION: >> /* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */ >> if (id >= BIT_ULL(16)) >> - return false; >> + return -EINVAL; >> break; >> default: >> - return false; >> + return -EINVAL; >> } >> >> if (!(baser & GITS_BASER_INDIRECT)) { >> phys_addr_t addr; >> >> if (id >= (l1_tbl_size / esz)) >> - return false; >> + return -EINVAL; >> >> addr = BASER_ADDRESS(baser) + id * esz; >> gfn = addr >> PAGE_SHIFT; >> >> if (eaddr) >> *eaddr = addr; >> - return kvm_is_visible_gfn(its->dev->kvm, gfn); >> + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) >> + return 0; >> + else >> + return -EFAULT; >> } >> >> /* calculate and check the index into the 1st level */ >> index = id / (SZ_64K / esz); >> if (index >= (l1_tbl_size / sizeof(u64))) >> - return false; >> + return -EINVAL; >> >> /* Each 1st level entry is represented by a 64-bit value. */ >> if (kvm_read_guest(its->dev->kvm, >> BASER_ADDRESS(baser) + index * sizeof(indirect_ptr), >> &indirect_ptr, sizeof(indirect_ptr))) >> - return false; >> + return -EFAULT; >> >> indirect_ptr = le64_to_cpu(indirect_ptr); >> >> /* check the valid bit of the first level entry */ >> if (!(indirect_ptr & BIT_ULL(63))) >> - return false; >> + return -EFAULT; >> >> /* >> * Mask the guest physical address and calculate the frame number. >> @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, >> >> if (eaddr) >> *eaddr = indirect_ptr; >> - return kvm_is_visible_gfn(its->dev->kvm, gfn); >> + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) >> + return 0; >> + else >> + return -EFAULT; >> } >> >> static int vgic_its_alloc_collection(struct vgic_its *its, >> @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its, >> { >> struct its_collection *collection; >> >> - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) >> + if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) >> return E_ITS_MAPC_COLLECTION_OOR; >> >> collection = kzalloc(sizeof(*collection), GFP_KERNEL); >> @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, >> gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); >> struct its_device *device; >> >> - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) >> + if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) >> return E_ITS_MAPD_DEVICE_OOR; >> >> if (valid && num_eventid_bits > VITS_TYPER_IDBITS) >> @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its) >> int ret; >> gpa_t eaddr; >> >> - if (!vgic_its_check_id(its, baser, >> - dev->device_id, &eaddr)) >> - return -EINVAL; >> + ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr); >> + if (ret) >> + return ret; >> >> ret = vgic_its_save_itt(its, dev); >> if (ret) >> -- >> 2.5.5 >>
On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote: > Hi, > > On 13/10/2017 15:16, Christoffer Dall wrote: > > On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote: > >> At the moment the device table save() returns -EINVAL if > >> vgic_its_check_id() fails to return the gpa of the entry > >> associated to the device/collection id. Let vgic_its_check_id() > >> return an int instead of a bool and return a more precised > >> error value: > >> - EINVAL in case the id is out of range > >> - EFAULT if the gpa is not provisionned or is not valid > >> > > > > This is just to ease debugging, yes? > > I understood user-space should be able to discriminate between bad guest > programming and values corrupted by the userspace (regs for instance). > In first case QEMU should not abort. In latter case it should abort. So what is userspace supposed to do in the first case? > > In vgic_its_check_id we are checking the L1 entry validity bit and in > case it is invalid we can't compute the GPA of the entry. I was thinking > we should return -EFAULT in that case. But maybe returning EFAULT in > case the BASER<n> address is not reachable also is wrong because that > may be caused by the userspace writing a wrong value. Sigh ... > I think if either userspace or the guest programmed something that cannot be traversed, then you just don't save/restore the ITS properly, because it's broken anyway, so I don't think we need to replicate the *same broken state* at the destination. Maybe I'm missing part of the picture here. Thanks, -Christoffer
Hi Christoffer, On 13/10/2017 19:56, Christoffer Dall wrote: > On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote: >> Hi, >> >> On 13/10/2017 15:16, Christoffer Dall wrote: >>> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote: >>>> At the moment the device table save() returns -EINVAL if >>>> vgic_its_check_id() fails to return the gpa of the entry >>>> associated to the device/collection id. Let vgic_its_check_id() >>>> return an int instead of a bool and return a more precised >>>> error value: >>>> - EINVAL in case the id is out of range >>>> - EFAULT if the gpa is not provisionned or is not valid >>>> >>> >>> This is just to ease debugging, yes? >> >> I understood user-space should be able to discriminate between bad guest >> programming and values corrupted by the userspace (regs for instance). >> In first case QEMU should not abort. In latter case it should abort. > > So what is userspace supposed to do in the first case? I was referring to https://www.spinics.net/lists/kvm/msg148791.html. QEMU is supposed to write a message in that case but not cause an abort(). This is what is actually implemented on QEMU side. In case the ioctl returns -EFAULT, we don't abort but simply warn. However at the moment we return -EINVAL in some circumstances where - I think - we should return -EFAULT. Hence this patch attempting to be more precise on the cause of the failure instead of abruptly returning -EINVAL here. Thanks Eric > >> >> In vgic_its_check_id we are checking the L1 entry validity bit and in >> case it is invalid we can't compute the GPA of the entry. I was thinking >> we should return -EFAULT in that case. But maybe returning EFAULT in >> case the BASER<n> address is not reachable also is wrong because that >> may be caused by the userspace writing a wrong value. Sigh ... >> > > I think if either userspace or the guest programmed something that > cannot be traversed, then you just don't save/restore the ITS properly, > because it's broken anyway, so I don't think we need to replicate the > *same broken state* at the destination. > > Maybe I'm missing part of the picture here. > > Thanks, > -Christoffer >
On Sat, Oct 14, 2017 at 10:52:45AM +0200, Auger Eric wrote: > Hi Christoffer, > > On 13/10/2017 19:56, Christoffer Dall wrote: > > On Fri, Oct 13, 2017 at 04:22:25PM +0200, Auger Eric wrote: > >> Hi, > >> > >> On 13/10/2017 15:16, Christoffer Dall wrote: > >>> On Wed, Sep 27, 2017 at 03:28:33PM +0200, Eric Auger wrote: > >>>> At the moment the device table save() returns -EINVAL if > >>>> vgic_its_check_id() fails to return the gpa of the entry > >>>> associated to the device/collection id. Let vgic_its_check_id() > >>>> return an int instead of a bool and return a more precised > >>>> error value: > >>>> - EINVAL in case the id is out of range > >>>> - EFAULT if the gpa is not provisionned or is not valid > >>>> > >>> > >>> This is just to ease debugging, yes? > >> > >> I understood user-space should be able to discriminate between bad guest > >> programming and values corrupted by the userspace (regs for instance). > >> In first case QEMU should not abort. In latter case it should abort. > > > > So what is userspace supposed to do in the first case? > > I was referring to https://www.spinics.net/lists/kvm/msg148791.html. > QEMU is supposed to write a message in that case but not cause an abort(). > > This is what is actually implemented on QEMU side. In case the ioctl > returns -EFAULT, we don't abort but simply warn. However at the moment > we return -EINVAL in some circumstances where - I think - we should > return -EFAULT. Hence this patch attempting to be more precise on the > cause of the failure instead of abruptly returning -EINVAL here. > ok, thanks makes sense. Thanks for sharing the background. -Christoffer
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 76bed2d..c1f7972 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its, } /* - * Check whether an ID can be stored into the corresponding guest table. + * vgic_its_check_id - Check whether an ID can be stored into + * the corresponding guest table. + * * For a direct table this is pretty easy, but gets a bit nasty for * indirect tables. We check whether the resulting guest physical address * is actually valid (covered by a memslot and guest accessible). * For this we have to read the respective first level entry. + * + * @its: its handle + * @baser: GITS_BASER<n> register + * @id: id of the device/collection + * @eaddr: output gpa of the corresponding table entry + * + * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if + * the address cannot be computed or is not valid */ -static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, - gpa_t *eaddr) +static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, + gpa_t *eaddr) { int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; u64 indirect_ptr, type = GITS_BASER_TYPE(baser); @@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, int index; gfn_t gfn; + if (!(baser & GITS_BASER_VALID)) + return -EFAULT; + switch (type) { case GITS_BASER_TYPE_DEVICE: if (id >= BIT_ULL(VITS_TYPER_DEVBITS)) - return false; + return -EINVAL; break; case GITS_BASER_TYPE_COLLECTION: /* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */ if (id >= BIT_ULL(16)) - return false; + return -EINVAL; break; default: - return false; + return -EINVAL; } if (!(baser & GITS_BASER_INDIRECT)) { phys_addr_t addr; if (id >= (l1_tbl_size / esz)) - return false; + return -EINVAL; addr = BASER_ADDRESS(baser) + id * esz; gfn = addr >> PAGE_SHIFT; if (eaddr) *eaddr = addr; - return kvm_is_visible_gfn(its->dev->kvm, gfn); + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) + return 0; + else + return -EFAULT; } /* calculate and check the index into the 1st level */ index = id / (SZ_64K / esz); if (index >= (l1_tbl_size / sizeof(u64))) - return false; + return -EINVAL; /* Each 1st level entry is represented by a 64-bit value. */ if (kvm_read_guest(its->dev->kvm, BASER_ADDRESS(baser) + index * sizeof(indirect_ptr), &indirect_ptr, sizeof(indirect_ptr))) - return false; + return -EFAULT; indirect_ptr = le64_to_cpu(indirect_ptr); /* check the valid bit of the first level entry */ if (!(indirect_ptr & BIT_ULL(63))) - return false; + return -EFAULT; /* * Mask the guest physical address and calculate the frame number. @@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id, if (eaddr) *eaddr = indirect_ptr; - return kvm_is_visible_gfn(its->dev->kvm, gfn); + if (kvm_is_visible_gfn(its->dev->kvm, gfn)) + return 0; + else + return -EFAULT; } static int vgic_its_alloc_collection(struct vgic_its *its, @@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its, { struct its_collection *collection; - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) + if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL)) return E_ITS_MAPC_COLLECTION_OOR; collection = kzalloc(sizeof(*collection), GFP_KERNEL); @@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its, gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd); struct its_device *device; - if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) + if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL)) return E_ITS_MAPD_DEVICE_OOR; if (valid && num_eventid_bits > VITS_TYPER_IDBITS) @@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its) int ret; gpa_t eaddr; - if (!vgic_its_check_id(its, baser, - dev->device_id, &eaddr)) - return -EINVAL; + ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr); + if (ret) + return ret; ret = vgic_its_save_itt(its, dev); if (ret)
At the moment the device table save() returns -EINVAL if vgic_its_check_id() fails to return the gpa of the entry associated to the device/collection id. Let vgic_its_check_id() return an int instead of a bool and return a more precised error value: - EINVAL in case the id is out of range - EFAULT if the gpa is not provisionned or is not valid We also check first the GITS_BASER<n> Valid bit is set. This allows the userspace to discriminate failure reasons. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- need to CC stable --- virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 17 deletions(-)