[v12,2/3] x86/sev: Change snp_guest_issue_request's fw_err

  The GHCB specification declares that the firmware error value for a
guest request will be stored in the lower 32 bits of EXIT_INFO_2.
The upper 32 bits are for the VMM's own error code. The fw_err argument
is thus a misnomer, and callers will need access to all 64 bits.

The type of unsigned long also causes problems, since sw_exit_info2 is
u64 (unsigned long long) vs the argument's unsigned long*. This type
is changed for issuing the guest request. The ioctl command struct's
error field is passed directly instead of a local variable, since an
incomplete guest request may not set the error code, and uninitialized
stack memory would be written back to user space.

The firmware might not even be called, so the call is bookended with
the no firmware call error and clearing the error.

Since the "fw_err" field is really exitinfo2 split into the upper bits'
vmm error code and lower bits' firmware error code, sev-guest.h is
updated to represent the 64 bit value as a union.

Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Peter Gonda <pgonda@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <Borislav.Petkov@amd.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Venu Busireddy <venu.busireddy@oracle.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Michael Sterritt <sterritt@google.com>

Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>
Reviewed-by: Borislav Petkov <Borislav.Petkov@amd.com>
Reviewed-by: Peter Gonda <pgonda@google.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 arch/x86/include/asm/sev-common.h       |  3 --
 arch/x86/include/asm/sev.h              |  4 +--
 arch/x86/kernel/sev.c                   | 13 ++++---
 drivers/virt/coco/sev-guest/sev-guest.c | 46 ++++++++++++-------------
 include/uapi/linux/sev-guest.h          | 18 ++++++++--
 5 files changed, 48 insertions(+), 36 deletions(-)
  

On Fri, Jan 20, 2023 at 09:48:55PM +0000, Dionna Glaze wrote:
> The GHCB specification declares that the firmware error value for a
> guest request will be stored in the lower 32 bits of EXIT_INFO_2.
> The upper 32 bits are for the VMM's own error code. The fw_err argument
> is thus a misnomer, and callers will need access to all 64 bits.
> 
> The type of unsigned long also causes problems, since sw_exit_info2 is
> u64 (unsigned long long) vs the argument's unsigned long*. This type
> is changed for issuing the guest request. The ioctl command struct's
> error field is passed directly instead of a local variable, since an
> incomplete guest request may not set the error code, and uninitialized
> stack memory would be written back to user space.
> 
> The firmware might not even be called, so the call is bookended with
> the no firmware call error and clearing the error.
> 
> Since the "fw_err" field is really exitinfo2 split into the upper bits'
> vmm error code and lower bits' firmware error code, sev-guest.h is
> updated to represent the 64 bit value as a union.
> 
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <Borislav.Petkov@amd.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Venu Busireddy <venu.busireddy@oracle.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
> Cc: Michael Sterritt <sterritt@google.com>
> 
> Fixes: d5af44dde546 ("x86/sev: Provide support for SNP guest request NAEs")
> Reviewed-by: Tom Lendacky <Thomas.Lendacky@amd.com>

> Reviewed-by: Borislav Petkov <Borislav.Petkov@amd.com>

Please don't add tags you haven't received. Especially a Reviewed-by. Have a
look here what they mean, if unsure:

Documentation/process/submitting-patches.rst

I've removed it - you don't have to resend just because of that.

Thx.
  

On Fri, Jan 20, 2023 at 09:48:55PM +0000, Dionna Glaze wrote:
> Since the "fw_err" field is really exitinfo2 split into the upper bits'
> vmm error code and lower bits' firmware error code, sev-guest.h is
> updated to represent the 64 bit value as a union.

Yah, documentation needs update too:

diff --git a/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
index 4f0dcc1d16e8..4f9df24b829f 100644
--- a/Documentation/virt/coco/sev-guest.rst
+++ b/Documentation/virt/coco/sev-guest.rst
@@ -57,9 +57,16 @@ counter (e.g. counter overflow), then -EIO will be returned.
                 __u64 req_data;
                 __u64 resp_data;
 
-                /* firmware error code on failure (see psp-sev.h) */
-                __u64 fw_err;
-        };
+                /* bits[63:32]: VMM error code, bits[31:0] firmware error code (see psp-sev.h) */
+                union {
+                        __u64 exitinfo2;
+                        struct {
+                                __u32 fw_error;
+                                __u32 vmm_error;
+                        };
+                };
+	};
+
 
 2.1 SNP_GET_REPORT
 ------------------

/me goes and looks at patch 3...

Yap, that change adding the union should all belong together in a single patch.

> @@ -366,24 +367,22 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>  		 * of the VMPCK and the error code being propagated back to the
>  		 * user as an ioctl() return code.
>  		 */
> -		rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
> +		rc = snp_issue_guest_request(exit_code, &snp_dev->input,
> +					     &arg->exitinfo2);
>  
>  		/*
>  		 * Override the error to inform callers the given extended
>  		 * request buffer size was too small and give the caller the
>  		 * required buffer size.
>  		 */
> -		err = SNP_GUEST_REQ_INVALID_LEN;
> +		arg->vmm_error = SNP_GUEST_VMM_ERR_INVALID_LEN;
>  		snp_dev->input.data_npages = certs_npages;
>  	}
>  
> -	if (fw_err)
> -		*fw_err = err;
> -
>  	if (rc) {
>  		dev_alert(snp_dev->dev,
> -			  "Detected error from ASP request. rc: %d, fw_err: %llu\n",
> -			  rc, *fw_err);
> +			  "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
> +			  rc, arg->exitinfo2);

I guess that's better off dumped in binary now:

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index d4973d2dbc24..a5d6ea3eebe9 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -380,7 +380,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 
 	if (rc) {
 		dev_alert(snp_dev->dev,
-			  "Detected error from ASP request. rc: %d, exitinfo2: %llu\n",
+			  "Detected error from ASP request. rc: %d, exitinfo2: 0x%llx\n",
 			  rc, arg->exitinfo2);
 		goto disable_vmpck;
 	}

Anyway, that's a lot of changes for a fix which needs to go to stable. I don't
mind them but not in a minimal fix.

So how is this for a minimal fix to go in now, ontop of your first patch? The
cleanups can then go later...

---
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 4ec4174e05a3..20b560a45bc1 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -322,7 +322,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
 				u8 type, void *req_buf, size_t req_sz, void *resp_buf,
 				u32 resp_sz, __u64 *fw_err)
 {
-	unsigned long err;
+	unsigned long err = SEV_RET_NO_FW_CALL;
 	u64 seqno;
 	int rc;
  

>
> Anyway, that's a lot of changes for a fix which needs to go to stable. I don't
> mind them but not in a minimal fix.
>

> So how is this for a minimal fix to go in now, ontop of your first patch? The
> cleanups can then go later...
>
> ---
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 4ec4174e05a3..20b560a45bc1 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -322,7 +322,7 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>                                 u8 type, void *req_buf, size_t req_sz, void *resp_buf,
>                                 u32 resp_sz, __u64 *fw_err)
>  {
> -       unsigned long err;
> +       unsigned long err = SEV_RET_NO_FW_CALL;
>         u64 seqno;
>         int rc;
>

This isn't the primary problem that needs fixing, although it is part
of it. The problem is that the host can provide a throttling error and
the guest will need to continue trying the exact same request or else
end up locking themself out of the vmpck due to the IV reuse patch
Peter sent.

I think Sean's request to keep throttling a host problem in user space
is not the right one in this case. That would avoid scheduling the
whole vCPU, but the guest code I'm proposing can do other useful work
while waiting. There will be no other code that depends on that
particular control flow.

If we call this one-liner the fix, then the VM guest still has the IV
reuse problem unless it can guarantee that globally no other thread
than the one requesting an attestation report will continue to retry
upon a throttling error in the exitinfo2 value. I wouldn't call that a
fix.
  

On Mon, Jan 23, 2023 at 01:22:07PM -0800, Dionna Amalie Glaze wrote:
> This isn't the primary problem that needs fixing, although it is part
> of it.

I'm replying to the 2/3 patch which is addressing this part.

> The problem is that the host can provide a throttling error and
> the guest will need to continue trying the exact same request or else
> end up locking themself out of the vmpck due to the IV reuse patch
> Peter sent.
> 
> I think Sean's request to keep throttling a host problem in user space

+ Sean.

> is not the right one in this case. That would avoid scheduling the
> whole vCPU, but the guest code I'm proposing can do other useful work
> while waiting. There will be no other code that depends on that
> particular control flow.

I know there's another issue but my angle is this:

You have a patch with Fixes tags and they all are fixing some shortcomings. Now,
those are good candidates for stable.

In order to be backportable to stable, those fixes should be as minimal as
possible so that a stable backport does not turn into a nightmare. By the looks
of it, your patches could be simplified to the bare minimum fixes. Cleanups and
improvements can go ontop.

That is, provided we want them in stable and by the looks of it, we probably do.

Which then means, you could do the minimal fixes first and then the cleanups
ontop.

Then, please try to structure your commit messages in a format close to:

Problem is A.

It happens because of B.

Fix it by doing C.

(Potentially do D).

I'm having hard time parsing

https://lore.kernel.org/all/20230120214857.835931-4-dionnaglaze@google.com/

Only in the second paragraph it is talking about what the problem is. And by the
looks of it, if the host throttles, then the guest should not return from that
request.

And adding module params for this are most of the time the wrong solution
because then people would have to *know* how to use them.

And before we add module params, the kernel should try to solve this without any
user interaction, if possible.

But I'm not 100% clear on what exactly we're fixing here so let's start first,
please, with a proper description of what the exact issue is and how it happens.

Thx.
  

On Tue, Jan 24, 2023, Borislav Petkov wrote:
> On Mon, Jan 23, 2023 at 01:22:07PM -0800, Dionna Amalie Glaze wrote:
> > This isn't the primary problem that needs fixing, although it is part
> > of it.
> 
> I'm replying to the 2/3 patch which is addressing this part.
> 
> > The problem is that the host can provide a throttling error and
> > the guest will need to continue trying the exact same request or else
> > end up locking themself out of the vmpck due to the IV reuse patch
> > Peter sent.
> > 
> > I think Sean's request to keep throttling a host problem in user space
> 
> + Sean.

It was not a request, it was a question[*].  In the same paragraph I even asked
if the guest was expected to schedule out the blocked task.

 : Why does throttling just punt back to the guest?  E.g. why not exit to userspace
 : and let userspace stall the vCPU?  Is the guest expected to schedule out the task
 : that's trying to make the request?

[*] https://lore.kernel.org/all/Y8rEFpbMV58yJIKy@google.com

> > is not the right one in this case. That would avoid scheduling the
> > whole vCPU, but the guest code I'm proposing can do other useful work
> > while waiting. There will be no other code that depends on that
> > particular control flow.

Relying on host behavior for this sort of thing is also a bad idea.   There is no
contract between the guest and host beyond the host being able to say "you've been
throttled".

Similar to Boris's comments about splitting this patch to fix bugs, separate the
handling of SNP_GUEST_VMM_ERR_BUSY from the internal ratelimiting.  For all intents
and purpsoses, gracefully handling SNP_GUEST_VMM_ERR_BUSY is mandatory.  Ratelimiting
requests from the guest side is a completely different story.