linux-kernel - Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

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]

Date:   Mon, 30 Oct 2017 12:49:14 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, 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 v6.1 16/38] crypto: ccp: Add Secure Encrypted
 Virtualization (SEV) command support

On 10/30/2017 12:21 PM, Borislav Petkov wrote:
...

> 
> Useless forward declarations.
> 

Actually its helpful in other patches. I was trying to avoid making too 
many code movement in other patches to eliminate the forward 
declarations. I guess I can fix in v7.

>>   static struct psp_device *psp_alloc_struct(struct sp_device *sp)
>>   {
>>   	struct device *dev = sp->dev;
> 
> ...
> 
>> +static int sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> 
> You can use the "__" prefix to denote that it is a lower-level helper:
> 
> __sev_do_cmd
> __sev_do_cmd_locked
> 
> Ditto for the other locked functions.

noted

> ...
> 
>> +static int sev_platform_init_locked(struct sev_data_init *data, int *error)
>> +{
>> +	struct psp_device *psp = psp_master;
>> +	struct sev_data_init *input = NULL;
>> +	int rc = 0;
>> +
>> +	if (!psp)
>> +		return -ENODEV;
>> +
>> +	if (psp->sev_state == SEV_STATE_INIT)
>> +		return 0;
>> +
>> +	if (!data) {
>> +		input = kzalloc(sizeof(*input), GFP_KERNEL);
>> +		if (!input)
>> +			return -ENOMEM;
>> +
>> +		data = input;
>> +	}
> 
> You can do the allocation in the enclosing function, outside of the
> critical region so that you can keep it shorter.
> 
> Or even better: if you're going to synchronize the commands with a
> mutex, you can define a static struct sev_data_init input in this file
> which you always hand in and then you can save yourself the kmalloc
> calls.
> 

If the buffer is allocated on the stack then there is no guarantee that 
__pa() will gives us a valid physical address. IIRC, when 
CONFIG_VMAP_STACK=y then stack space is mapped similar to vmalloc'd 
storage and __pa() will not work.

Since we need to pass the physical address to PSP hence variable 
allocated on the stack will not work.

I can certainly move the allocation outside, but then it may increase 
the code size in other functions. If its not a big deal then I would 
prefer to keep what we have.

...
>> +
>> +int sev_platform_shutdown(int *error)
>> +{
>> +	if (error)
>> +		*error = 0;
>> +
>> +	return 0;
>> +}
> 
> I'm guessing that that's just bare-bones and it will get filled up in
> the next patches. Otherwise it looks pretty useless.
> 

Well, we are not expanding in other patches. I was also debating on what 
to do with this function. Since we need sev_platform_init() hence it 
made sense to add sev_platform_shutdown() as well. If we add the 
function then I wanted to make sure that we set the *error = SUCCESS so 
that caller knows that function succeeded.

> If it is just to block the user from sending SHUTDOWN to the PSP
> master, just do that in the ioctl directly - no need to call some empty
> functions.
> 

The function is not used by userspace ioctl, its used by kvm drv when it 
launch/terminates the SEV guest.

-Brijesh