linux-kernel - Re: [PATCH RFC v8 12/56] x86/sev: Add RMP entry lookup helpers

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: <9c8829f6-8049-2c83-49a4-285757cc9259@suse.cz>
Date:   Thu, 20 Apr 2023 18:31:18 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Michael Roth <michael.roth@....com>
Cc:     kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
        linux-mm@...ck.org, linux-crypto@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        jroedel@...e.de, thomas.lendacky@....com, hpa@...or.com,
        ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
        vkuznets@...hat.com, jmattson@...gle.com, luto@...nel.org,
        dave.hansen@...ux.intel.com, slp@...hat.com, pgonda@...gle.com,
        peterz@...radead.org, srinivas.pandruvada@...ux.intel.com,
        rientjes@...gle.com, dovmurik@...ux.ibm.com, tobin@....com,
        bp@...en8.de, kirill@...temov.name, ak@...ux.intel.com,
        tony.luck@...el.com, marcorr@...gle.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com, alpergun@...gle.com,
        dgilbert@...hat.com, jarkko@...nel.org, ashish.kalra@....com,
        nikunj.dadhania@....com, Brijesh Singh <brijesh.singh@....com>
Subject: Re: [PATCH RFC v8 12/56] x86/sev: Add RMP entry lookup helpers

On 3/30/23 00:59, Michael Roth wrote:
> On Fri, Mar 03, 2023 at 04:28:39PM +0100, Vlastimil Babka wrote:
>> On 2/20/23 19:38, Michael Roth wrote:
>> > From: Brijesh Singh <brijesh.singh@....com>
>> > 
>> > The snp_lookup_page_in_rmptable() can be used by the host to read the RMP
>> > entry for a given page. The RMP entry format is documented in AMD PPR, see
>> > https://bugzilla.kernel.org/attachment.cgi?id=296015.
>> > 
>> > Co-developed-by: Ashish Kalra <ashish.kalra@....com>
>> > Signed-off-by: Ashish Kalra <ashish.kalra@....com>
>> > Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> > Signed-off-by: Michael Roth <michael.roth@....com>
>> > ---
>> 
>> > +/*
>> > + * Return 1 if the RMP entry is assigned, 0 if it exists but is not assigned,
>> > + * and -errno if there is no corresponding RMP entry.
>> > + */
>> 
>> Hmm IMHO the kernel's idiomatic way is to return 0 on "success" and I'd
>> assume the more intuitive expectation of success here if the entry is
>> assigned?
> 
> In general I'd agree. Here's it's a little awkward though.
> snp_lookup_rmpentry() sort of wants to be a bool, where true indicates
> an assigned entry was found, false indicates no assigned entry.
> 
> But it also has to deal with error values, so the most direct way to
> encapsulate that is true == 1, false == 0, and < 0 for errors.
> 
> Inverting it to align more with kernel expections of 0 == success/true
> gets awkward too, because stuff like:
> 
>   if (snp_lookup_rmpentry(...))
>     //error
> 
> still doesn't work the way most other functions written in this way
> would since it could still be "successful" if we were expecting PFN to
> be in shared state. So the return value needs special handling there
> too.
> 
> Would it make sense to define it something like this?:
> 
>   /*
>    * Query information about the RMP entry corresponding to the given
>    * PFN.
>    * 
>    * Returns 0 on success, and -errno if there was a problem accessing
>    * the RMP entry.
>    */
>   int snp_lookup_rmpentry(u64 pfn, int *level, bool *assigned)

Yeah that looks fine to me. Hope you find out it makes it easier to work
with in the callers too.

> 
>> The various callers seem to differ though so I guess it depends on
>> context. Some however don't distinguish their "failure" from an ERR and
>> maybe they should, at least for the purposes of the various printks?
> 
> Yes, regardless of what we decide above, the call-sites should properly
> distinguish between failure/assigned/not-assigned and report the
> information accordingly. I'll get those fixed up where needed.

Great, thanks!

> Thanks,
> 
> -Mike
> 
>> 
>> > +int snp_lookup_rmpentry(u64 pfn, int *level)
>> > +{
>> > +	struct rmpentry *e;
>> > +
>> > +	e = __snp_lookup_rmpentry(pfn, level);
>> > +	if (IS_ERR(e))
>> > +		return PTR_ERR(e);
>> > +
>> > +	return !!rmpentry_assigned(e);
>> > +}
>> > +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);
>>