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
| ||
|
Date: Wed, 16 Nov 2022 10:10:58 -0700 From: Peter Gonda <pgonda@...gle.com> To: Tom Lendacky <thomas.lendacky@....com> Cc: Borislav Petkov <bp@...e.de>, Dionna Glaze <dionnaglaze@...gle.com>, Michael Roth <michael.roth@....com>, Haowen Bai <baihaowen@...zu.com>, Yang Yingliang <yangyingliang@...wei.com>, Marc Orr <marcorr@...gle.com>, David Rientjes <rientjes@...gle.com>, Ashish Kalra <Ashish.Kalra@....com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org Subject: Re: [PATCH V4] virt: sev: Prevent IV reuse in SNP guest driver On Wed, Nov 16, 2022 at 9:58 AM Tom Lendacky <thomas.lendacky@....com> wrote: > > On 11/16/22 10:23, Peter Gonda wrote: > > On Wed, Nov 16, 2022 at 5:20 AM Borislav Petkov <bp@...e.de> wrote: > >> > >> On Tue, Nov 15, 2022 at 02:47:31PM -0700, Peter Gonda wrote: > >>>>> + * certificate data buffer retry the same guest request without the > >>>>> + * extended data request. > >>>>> + */ > >>>>> + if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST && > >>>>> + err == SNP_GUEST_REQ_INVALID_LEN) { > >>>>> + const unsigned int certs_npages = snp_dev->input.data_npages; > >>>>> + > >>>>> + exit_code = SVM_VMGEXIT_GUEST_REQUEST; > >>>>> + rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > >>>>> + > >>>>> + err = SNP_GUEST_REQ_INVALID_LEN; > >>>> > >>>> Huh, why are we overwriting err here? > >>> > >>> I have added a comment for the next revision. > >>> > >>> We are overwriting err here so that userspace is alerted that they > >>> supplied a buffer too small. > >> > >> Sure but you're not checking rc either. What if that reissue fails for > >> whatever other reason? -EIO for example... > > > > If we get any error here we have to wipe the VMPCK here so I thought > > More accurate to say that you will wipe the VMPCK, since the value of rc > is checked a bit further down in the code and the -EIO (or other non-zero) > will be result in a call to snp_disable_vmpck() and rc being propagated > back to the user as an ioctl() return code. > > Might be worth a comment above that second snp_issue_guest_request() > explaining that. I'll add a comment above the second snp_issue_guest_request(), good idea thanks. I think another comment above the first snp_issue_guest_request() could help too. Saying once we call this function we either need to increment the sequence number or wipe the VMPCK to ensure the encryption scheme is safe. > > > this always override @err was OK. > > > > I can update this to only override @err if after the secondary > > SVM_VMGEXIT_GUEST_REQUEST rc and err are OK. Thoughts? > > I think it's ok to set it no matter what, but I don't have a strong > opinion either way. > > Thanks, > Tom > > > > >> > >> -- > >> Regards/Gruss, > >> Boris. > >> > >> SUSE Software Solutions Germany GmbH > >> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman > >> (HRB 36809, AG Nürnberg)
Powered by blists - more mailing lists