On Mon, Jul 2, 2018 at 12:36 AM, Auger Eric <eric.auger@xxxxxxxxxx> wrote: > Hi Kees, > > On 06/29/2018 08:46 PM, Kees Cook wrote: >> In the quest to remove all stack VLA usage from the kernel[1], this >> switches to using a maximum size and adds sanity checks. Additionally >> cleans up some of the int-vs-u32 usage and adds additional bounds checking. >> As it currently stands, this will always be 8 bytes until the ABI changes. >> >> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@xxxxxxxxxxxxxx >> >> Cc: Christoffer Dall <christoffer.dall@xxxxxxx> >> Cc: Marc Zyngier <marc.zyngier@xxxxxxx> >> Cc: Eric Auger <eric.auger@xxxxxxxxxx> >> Cc: Andre Przywara <andre.przywara@xxxxxxx> >> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >> Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 19 +++++++++++++++---- >> 1 file changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 4ed79c939fb4..3143fc047fcf 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -168,8 +168,14 @@ struct vgic_its_abi { >> int (*commit)(struct vgic_its *its); >> }; >> >> +#define ABI_0_ESZ 8 >> +#define ESZ_MAX ABI_0_ESZ >> + >> static const struct vgic_its_abi its_table_abi_versions[] = { >> - [0] = {.cte_esz = 8, .dte_esz = 8, .ite_esz = 8, >> + [0] = { >> + .cte_esz = ABI_0_ESZ, >> + .dte_esz = ABI_0_ESZ, >> + .ite_esz = ABI_0_ESZ, >> .save_tables = vgic_its_save_tables_v0, >> .restore_tables = vgic_its_restore_tables_v0, >> .commit = vgic_its_commit_v0, >> @@ -180,10 +186,12 @@ static const struct vgic_its_abi its_table_abi_versions[] = { >> >> inline const struct vgic_its_abi *vgic_its_get_abi(struct vgic_its *its) >> { >> + if (WARN_ON(its->abi_rev >= NR_ITS_ABIS)) >> + return NULL; >> return &its_table_abi_versions[its->abi_rev]; >> } >> >> -int vgic_its_set_abi(struct vgic_its *its, int rev) >> +static int vgic_its_set_abi(struct vgic_its *its, u32 rev) >> { > if vgic_its_get_abi is likely to return NULL, don't we need to check abi > != NULL in all call sites. My thinking was that since it should never happen, a WARN_ON would be sufficient. But I can drop all these changes if you want. I just wanted to see the VLA removed. :) -Kees > > abi_rev is actually set by vgic_its_set_abi() which is actually called > by vgic_mmio_uaccess_write_its_iidr() and vgic_its_create(). > > Only vgic_mmio_uaccess_write_its_iidr allows the userspace to overwrite > the default abi_rev. At this point a check against NR_ITS_ABIS is > already done. So to me the check is done at the source? > > Thanks > > Eric >> const struct vgic_its_abi *abi; >> >> @@ -1881,16 +1889,19 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry, >> * Return: < 0 on error, 0 if last element was identified, 1 otherwise >> * (the last element may not be found on second level tables) >> */ >> -static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz, >> +static int scan_its_table(struct vgic_its *its, gpa_t base, int size, u32 esz, >> int start_id, entry_fn_t fn, void *opaque) >> { >> struct kvm *kvm = its->dev->kvm; >> unsigned long len = size; >> int id = start_id; >> gpa_t gpa = base; >> - char entry[esz]; >> + char entry[ESZ_MAX]; >> int ret; >> >> + if (WARN_ON(esz > ESZ_MAX)) >> + return -EINVAL; >> + >> memset(entry, 0, esz); >> >> while (len > 0) { >> -- Kees Cook Pixel Security _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
