linux-kernel - Re: [RFC V3 PATCH 6/6] sefltests: kvm: x86: Add selftest for private memory

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAGtprH-eA+k3BwczSyds+Hrr5QZn96hNK81Op_iBH20-wKfKeg@mail.gmail.com>
Date:   Fri, 14 Oct 2022 15:11:19 +0530
From:   Vishal Annapurve <vannapurve@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, pbonzini@...hat.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        joro@...tes.org, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        shuah@...nel.org, yang.zhong@...el.com, drjones@...hat.com,
        ricarkol@...gle.com, aaronlewis@...gle.com, wei.w.wang@...el.com,
        kirill.shutemov@...ux.intel.com, corbet@....net, hughd@...gle.com,
        jlayton@...nel.org, bfields@...ldses.org,
        akpm@...ux-foundation.org, chao.p.peng@...ux.intel.com,
        yu.c.zhang@...ux.intel.com, jun.nakajima@...el.com,
        dave.hansen@...el.com, michael.roth@....com, qperret@...gle.com,
        steven.price@....com, ak@...ux.intel.com, david@...hat.com,
        luto@...nel.org, vbabka@...e.cz, marcorr@...gle.com,
        erdemaktas@...gle.com, pgonda@...gle.com, nikunj@....com,
        diviness@...gle.com, maz@...nel.org, dmatlack@...gle.com,
        axelrasmussen@...gle.com, maciej.szmigiero@...cle.com,
        mizhang@...gle.com, bgardon@...gle.com
Subject: Re: [RFC V3 PATCH 6/6] sefltests: kvm: x86: Add selftest for private memory

On Fri, Oct 7, 2022 at 1:54 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Fri, Aug 19, 2022, Vishal Annapurve wrote:
> > +static bool verify_mem_contents(void *mem, uint32_t size, uint8_t pat)
>
> As per feedback in v1[*], spell out "pattern".
>
> [*] https://lore.kernel.org/all/YtiJx11AZHslcGnN@google.com
>
> > +{
> > +     uint8_t *buf = (uint8_t *)mem;
> > +
> > +     for (uint32_t i = 0; i < size; i++) {
> > +             if (buf[i] != pat)
> > +                     return false;
> > +     }
> > +
> > +     return true;
> > +}
> > +
> > +/*
> > + * Add custom implementation for memset to avoid using standard/builtin memset
> > + * which may use features like SSE/GOT that don't work with guest vm execution
> > + * within selftests.
> > + */
> > +void *memset(void *mem, int byte, size_t size)
> > +{
> > +     uint8_t *buf = (uint8_t *)mem;
> > +
> > +     for (uint32_t i = 0; i < size; i++)
> > +             buf[i] = byte;
> > +
> > +     return buf;
> > +}
>
> memset(), memcpy(), and memcmp() are safe to use as of commit 6b6f71484bf4 ("KVM:
> selftests: Implement memcmp(), memcpy(), and memset() for guest use").
>

This is much better. It made less sense to add a custom memset for a
single selftest.

> Note the "fun" with gcc "optimizing" into infinite recursion... :-)
>
> > +
> > +static void populate_test_area(void *test_area_base, uint64_t pat)
> > +{
> > +     memset(test_area_base, pat, TEST_AREA_SIZE);
> > +}
> > +
> > +static void populate_guest_test_mem(void *guest_test_mem, uint64_t pat)
> > +{
> > +     memset(guest_test_mem, pat, GUEST_TEST_MEM_SIZE);
> > +}
> > +
> > +static bool verify_test_area(void *test_area_base, uint64_t area_pat,
> > +     uint64_t guest_pat)
>
> Again, avoid "pat".
>
> > +{
> > +     void *test_area1_base = test_area_base;
> > +     uint64_t test_area1_size = GUEST_TEST_MEM_OFFSET;
> > +     void *guest_test_mem = test_area_base + test_area1_size;
> > +     uint64_t guest_test_size = GUEST_TEST_MEM_SIZE;
> > +     void *test_area2_base = guest_test_mem + guest_test_size;
> > +     uint64_t test_area2_size = (TEST_AREA_SIZE - (GUEST_TEST_MEM_OFFSET +
> > +                     GUEST_TEST_MEM_SIZE));
>
> This is all amazingly hard to read.  AFAICT, the local variables are largely useless.
> Actually, why even take in @test_area_base, isn't it hardcoded to TEST_AREA_GPA?
> Then everything except the pattern can be hardcoded.
>
> > +     return (verify_mem_contents(test_area1_base, test_area1_size, area_pat) &&
> > +             verify_mem_contents(guest_test_mem, guest_test_size, guest_pat) &&
> > +             verify_mem_contents(test_area2_base, test_area2_size, area_pat));
> > +}

Ack. Will address these comments in the next series.