linux-kernel - Re: [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest

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: <32181e52-da8e-404c-9832-cb29e9e31064@amd.com>
Date: Wed, 17 Apr 2024 09:48:09 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
 kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
 dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
 pbonzini@...hat.com
Subject: Re: [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev
 struct

On 4/16/2024 2:36 PM, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 11:27:24AM +0530, Nikunj A. Dadhania wrote:
>>> Why does that snp_get_os_area_msg_seqno() returns a pointer when you
>>> deref it here again?
>>>
>>> A function which returns a sequence number should return that number
>>> - not a pointer to it.
>>>
>>> Which then makes that u32 *os_area_msg_seqno redundant and you can use
>>> the function directly.
>>>
>>> IOW:
>>>
>>> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
>>> {
>>>         return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id;
>>
>> This patch removes setting of layour page in snp_dev structure.
> 
> So?

* Instead of using snp_dev->layout, we will need to access it using platform_data->layout structure.
* Below will give incorrect value of sequence number, it will get VMPCK_0's sequence number and will add vmpck_id to that. Will work by fluke for VMPCK=0, but will fail for all other keys.

  return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id;


struct secrets_os_area {
..
        u32 msg_seqno_0;
        u32 msg_seqno_1;
        u32 msg_seqno_2;
        u32 msg_seqno_3;
..
}

* I am using vmpck_id to index to correct msg_seqno_*


Changing this to

struct secrets_os_area {
..
        u32 msg_seqno[VMPCK_MAX_NUM];
..
}


> 
>> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
>> {
>>         if (!platform_data)
>>                 return NULL;
>>
>>         return *(&platform_data->layout->os_area.msg_seqno_0 + vmpck_id);
>> }
> 
> What?!

I can change the secrets_os_area like below to simplify things:

struct secrets_os_area {
..
        u32 msg_seqno[VMPCK_MAX_NUM];
..
}


static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
{
         if (!platform_data)
                 return NULL;

         return platform_data->layout->os_area.msg_seqno[snp_dev->vmpck_id];
}


> 
> This snp_get_os_area_msg_seqno() is a new function added by this patch.
> 
>> I had a getter for getting the os_area_msg_seqno pointer, probably not
>> a good function name.
> 
> Probably you need to go back to the drawing board and think about how
> this thing should look like.
> 
>>> Do you see the imbalance in the APIs?
>>
>> The msg_seqno should only be incremented by 2 (always), that was the reason to avoid a setter.
> 
> And what's wrong with the setter doing the incrementation so that
> callers can't even get it wrong?

Are you suggesting that setter should always increment by 2?

static inline u32 snp_set_os_area_msg_seqno(struct snp_guest_dev *snp_dev)
{
..
	os_area.msg_seqno[snp_dev->vmpck_id] += 2;
..

}

> 
> It sounds to me like you should redesign this sequence number handling
> in a *separate* patch.

Sure, let me rethink and will post it as separate patch.

Regards
Nikunj