Re: [PATCH] KVM: arm64: vgic-its: Remove VLA usage

[Kees Cook]

On Mon, Jul 9, 2018 at 3:47 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Hi kees,
>
> On 02/07/18 18:15, Kees Cook wrote:
>> 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. :)
>
> Are you going to respin this patch limiting it to just the VLA changes?
> I'm actively queuing stuff for the next merge window, and it'd be good
> to get that one in.
>
> Alternatively, I can drop the WARN_ONs myself. Just let me know.

If you can just drop them that would be best, thanks!

-Kees

-- 
Kees Cook
Pixel Security