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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f213e03a-66a6-0c07-7bb8-2061b9c24731@arm.com>
Date:   Mon, 9 Jul 2018 11:47:44 +0100
From:   Marc Zyngier <marc.zyngier@....com>
To:     Kees Cook <keescook@...omium.org>,
        Auger Eric <eric.auger@...hat.com>
Cc:     Christoffer Dall <christoffer.dall@....com>,
        Andre Przywara <andre.przywara@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        kvmarm@...ts.cs.columbia.edu,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] KVM: arm64: vgic-its: Remove VLA usage

Hi kees,

On 02/07/18 18:15, Kees Cook wrote:
> On Mon, Jul 2, 2018 at 12:36 AM, Auger Eric <eric.auger@...hat.com> 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@mail.gmail.com
>>>
>>> Cc: Christoffer Dall <christoffer.dall@....com>
>>> Cc: Marc Zyngier <marc.zyngier@....com>
>>> Cc: Eric Auger <eric.auger@...hat.com>
>>> Cc: Andre Przywara <andre.przywara@....com>
>>> Cc: linux-arm-kernel@...ts.infradead.org
>>> Cc: kvmarm@...ts.cs.columbia.edu
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>>  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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...