linux-kernel - Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM

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: <20171011202836.d3ur3ndrnea25ywi@pd.tnic>
Date:   Wed, 11 Oct 2017 22:28:36 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Gary Hook <gary.hook@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        linux-crypto@...r.kernel.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement
 SEV_PLATFORM_STATUS ioctl command

On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote:
> The current 'struct sev_data_status' matches with the firmware names and the
> bit fields. Only thing I did was the fields with no name is called as
> "reservedX"

Ok, I see it. So what you actually wanna do is:

struct sev_data_status {
        u8 api_major;                           /* Out */
        u8 api_minor;                           /* Out */
        u8 state;                               /* Out */
        u8 flags;                               /* Out */
        u32 config;                             /* Out */
        u32 guest_count;                        /* Out */
} __packed;

as this is exactly what the firwmare gives you. Theoretically, you
could've also done:

struct sev_data_status {
	u64 first_qword;
	u32 second_dword;
};

but you have the fields mostly defined already and that would be too
confusing.

What I mean is, once you've gotten the command buffer, then you can pick
fields apart in software.

	owner	  = status.flags & 1;
	config_es = status.config & 1;
	build	  = (status.config >> 24) & 0xff;

This way, if new fields get added, you don't have to change the struct
definitions - *especially* if they're visible to userspace - and users
of that struct can be extended to understand the new fields.

And before you copy the struct to userspace, you can simply clear out
the reserved fields as nothing should rely on them having a particular
value, because, well, they're reserved, doh.

Makes sense?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--