linux-kernel - Re: [PATCH v8 06/16] virt: sev-guest: Move SNP Guest command mutex

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: <f8dbd58c-78da-4b3f-a79b-6693c04fb104@amd.com>
Date: Tue, 23 Apr 2024 16:12:00 +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 06/16] virt: sev-guest: Move SNP Guest command mutex

On 4/23/2024 3:58 PM, Borislav Petkov wrote:
> On Tue, Apr 23, 2024 at 09:52:41AM +0530, Nikunj A. Dadhania wrote:
>> SNP guest messaging will be moving as part of sev.c, and Secure TSC code
>> will use this mutex.
> 
> No, this is all backwards.
> 
> You have a *static* function in sev-guest - snp_guest_ioctl- which takes
> an exported lock - snp_guest_cmd_lock - in order to synchronize with
> other callers which are only in that same sev-guest driver.
> 
> Why do you even need the guest messaging in sev.c?
> 
> I guess this: "Many of the required functions are implemented in the
> sev-guest driver and therefore not available at early boot."

Yes.

> 
> But then your API is misdesigned: the lock should be private to sev.c
> and none of the callers should pay attention to grabbing it - the
> callers simply call the functions and underneath the locking works
> automatically for them - they don't care. Just like any other shared
> resource, users see only the API they call and the actual
> synchronization is done behind the scenes.
>
> Sounds like you need to go back to the drawing board and think how this
> thing should look like.

Something like below ?

snp_guest_ioctl()
-> get_report()/get_derived_key()/get_ext_report()
  -> snp_send_guest_request()
       snp_guest_cmd_lock();
       ...
       snp_guest_cmd_lock();

With this the cmd_lock will be private to sev.c and lock/unlock function
doesn't need to be exported.

Regards
Nikunj