| Message ID | 20220427184814.2204513-3-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: Misc ITS fixes | expand |
Hi Ricardo, On 4/27/22 20:48, Ricardo Koller wrote: > Try to improve the predictability of ITS save/restores (and debuggability > of failed ITS saves) by failing early on restore when trying to read > corrupted tables. > > Restoring the ITS tables does some checks for corrupted tables, but not as > many as in a save: an overflowing device ID will be detected on save but > not on restore. The consequence is that restoring a corrupted table won't > be detected until the next save; including the ITS not working as expected > after the restore. As an example, if the guest sets tables overlapping > each other, which would most likely result in some corrupted table, this is > what we would see from the host point of view: > > guest sets base addresses that overlap each other > save ioctl > restore ioctl > save ioctl (fails) > > Ideally, we would like the first save to fail, but overlapping tables could > actually be intended by the guest. So, let's at least fail on the restore > with some checks: like checking that device and event IDs don't overflow > their tables. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index e14790750958..fb2d26a73880 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id, > if (!collection) > return -EINVAL; > > + if (find_ite(its, dev->device_id, event_id)) > + return -EINVAL; Unsure about that. Nothing in the arm-vgic-its.rst doc says that the KVM_DEV_ARM_ITS_RESTORE_TABLES ioctl cannot be called several times (although obviously useless) > + > + if (!vgic_its_check_event_id(its, dev, event_id)) > + return -EINVAL; > + > ite = vgic_its_alloc_ite(dev, collection, event_id); > if (IS_ERR(ite)) > return PTR_ERR(ite); > @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, > void *ptr, void *opaque) > { > struct its_device *dev; > + u64 baser = its->baser_device_table; > gpa_t itt_addr; > u8 num_eventid_bits; > u64 entry = *(u64 *)ptr; > @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, > /* dte entry is valid */ > offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT; > > + if (find_its_device(its, id)) > + return -EINVAL; same here. > + > + if (!vgic_its_check_id(its, baser, id, NULL)) > + return -EINVAL; > + > dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits); > if (IS_ERR(dev)) > return PTR_ERR(dev); Thanks Eric
On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote: > Hi Ricardo, > > On 4/27/22 20:48, Ricardo Koller wrote: > > Try to improve the predictability of ITS save/restores (and debuggability > > of failed ITS saves) by failing early on restore when trying to read > > corrupted tables. > > > > Restoring the ITS tables does some checks for corrupted tables, but not as > > many as in a save: an overflowing device ID will be detected on save but > > not on restore. The consequence is that restoring a corrupted table won't > > be detected until the next save; including the ITS not working as expected > > after the restore. As an example, if the guest sets tables overlapping > > each other, which would most likely result in some corrupted table, this is > > what we would see from the host point of view: > > > > guest sets base addresses that overlap each other > > save ioctl > > restore ioctl > > save ioctl (fails) > > > > Ideally, we would like the first save to fail, but overlapping tables could > > actually be intended by the guest. So, let's at least fail on the restore > > with some checks: like checking that device and event IDs don't overflow > > their tables. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > > index e14790750958..fb2d26a73880 100644 > > --- a/arch/arm64/kvm/vgic/vgic-its.c > > +++ b/arch/arm64/kvm/vgic/vgic-its.c > > @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id, > > if (!collection) > > return -EINVAL; > > > > + if (find_ite(its, dev->device_id, event_id)) > > + return -EINVAL; > Unsure about that. Nothing in the arm-vgic-its.rst doc says that the > KVM_DEV_ARM_ITS_RESTORE_TABLES ioctl cannot be called several times > (although obviously useless) In that case, maybe we could ignore the new repeated entry? or overwrite the old one? find_ite() only returns the first (device_id, event_id) match. So, it's like the new one is ignored already. The arm arm says this about MAPI commands in this situation: If there is an existing mapping for the EventID-DeviceID combination, behavior is UNPREDICTABLE. And, just in case, the main reason for adding this check was to avoid failing the next ITS save. The idea is to try to fail as soon as possible, not in possibly many days during the next migration attempt. > > + > > + if (!vgic_its_check_event_id(its, dev, event_id)) > > + return -EINVAL; > > + > > ite = vgic_its_alloc_ite(dev, collection, event_id); > > if (IS_ERR(ite)) > > return PTR_ERR(ite); > > @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, > > void *ptr, void *opaque) > > { > > struct its_device *dev; > > + u64 baser = its->baser_device_table; > > gpa_t itt_addr; > > u8 num_eventid_bits; > > u64 entry = *(u64 *)ptr; > > @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, > > /* dte entry is valid */ > > offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT; > > > > + if (find_its_device(its, id)) > > + return -EINVAL; > same here. > > + > > + if (!vgic_its_check_id(its, baser, id, NULL)) > > + return -EINVAL; > > + > > dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits); > > if (IS_ERR(dev)) > > return PTR_ERR(dev); > Thanks > > Eric > Thanks, Ricardo
Hi Ricardo, On 5/4/22 19:01, Ricardo Koller wrote: > On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote: >> Hi Ricardo, >> >> On 4/27/22 20:48, Ricardo Koller wrote: >>> Try to improve the predictability of ITS save/restores (and debuggability >>> of failed ITS saves) by failing early on restore when trying to read >>> corrupted tables. >>> >>> Restoring the ITS tables does some checks for corrupted tables, but not as >>> many as in a save: an overflowing device ID will be detected on save but >>> not on restore. The consequence is that restoring a corrupted table won't >>> be detected until the next save; including the ITS not working as expected >>> after the restore. As an example, if the guest sets tables overlapping >>> each other, which would most likely result in some corrupted table, this is >>> what we would see from the host point of view: >>> >>> guest sets base addresses that overlap each other >>> save ioctl >>> restore ioctl >>> save ioctl (fails) >>> >>> Ideally, we would like the first save to fail, but overlapping tables could >>> actually be intended by the guest. So, let's at least fail on the restore >>> with some checks: like checking that device and event IDs don't overflow >>> their tables. >>> >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> >>> --- >>> arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c >>> index e14790750958..fb2d26a73880 100644 >>> --- a/arch/arm64/kvm/vgic/vgic-its.c >>> +++ b/arch/arm64/kvm/vgic/vgic-its.c >>> @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id, >>> if (!collection) >>> return -EINVAL; >>> >>> + if (find_ite(its, dev->device_id, event_id)) >>> + return -EINVAL; >> Unsure about that. Nothing in the arm-vgic-its.rst doc says that the >> KVM_DEV_ARM_ITS_RESTORE_TABLES ioctl cannot be called several times >> (although obviously useless) > In that case, maybe we could ignore the new repeated entry? or Maybe you can fail only in the case the ITE to be restored is different from the existing one? otherwise ignore. Eric > overwrite the old one? find_ite() only returns the first (device_id, > event_id) match. So, it's like the new one is ignored already. The > arm arm says this about MAPI commands in this situation: > > If there is an existing mapping for the EventID-DeviceID > combination, behavior is UNPREDICTABLE. > > And, just in case, the main reason for adding this check was to avoid > failing the next ITS save. The idea is to try to fail as soon as > possible, not in possibly many days during the next migration attempt. > >>> + >>> + if (!vgic_its_check_event_id(its, dev, event_id)) >>> + return -EINVAL; >>> + >>> ite = vgic_its_alloc_ite(dev, collection, event_id); >>> if (IS_ERR(ite)) >>> return PTR_ERR(ite); >>> @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, >>> void *ptr, void *opaque) >>> { >>> struct its_device *dev; >>> + u64 baser = its->baser_device_table; >>> gpa_t itt_addr; >>> u8 num_eventid_bits; >>> u64 entry = *(u64 *)ptr; >>> @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, >>> /* dte entry is valid */ >>> offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT; >>> >>> + if (find_its_device(its, id)) >>> + return -EINVAL; >> same here. >>> + >>> + if (!vgic_its_check_id(its, baser, id, NULL)) >>> + return -EINVAL; >>> + >>> dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits); >>> if (IS_ERR(dev)) >>> return PTR_ERR(dev); >> Thanks >> >> Eric >> > Thanks, > Ricardo >
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index e14790750958..fb2d26a73880 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id, if (!collection) return -EINVAL; + if (find_ite(its, dev->device_id, event_id)) + return -EINVAL; + + if (!vgic_its_check_event_id(its, dev, event_id)) + return -EINVAL; + ite = vgic_its_alloc_ite(dev, collection, event_id); if (IS_ERR(ite)) return PTR_ERR(ite); @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, void *ptr, void *opaque) { struct its_device *dev; + u64 baser = its->baser_device_table; gpa_t itt_addr; u8 num_eventid_bits; u64 entry = *(u64 *)ptr; @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id, /* dte entry is valid */ offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT; + if (find_its_device(its, id)) + return -EINVAL; + + if (!vgic_its_check_id(its, baser, id, NULL)) + return -EINVAL; + dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits); if (IS_ERR(dev)) return PTR_ERR(dev);
Try to improve the predictability of ITS save/restores (and debuggability of failed ITS saves) by failing early on restore when trying to read corrupted tables. Restoring the ITS tables does some checks for corrupted tables, but not as many as in a save: an overflowing device ID will be detected on save but not on restore. The consequence is that restoring a corrupted table won't be detected until the next save; including the ITS not working as expected after the restore. As an example, if the guest sets tables overlapping each other, which would most likely result in some corrupted table, this is what we would see from the host point of view: guest sets base addresses that overlap each other save ioctl restore ioctl save ioctl (fails) Ideally, we would like the first save to fail, but overlapping tables could actually be intended by the guest. So, let's at least fail on the restore with some checks: like checking that device and event IDs don't overflow their tables. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)