The err variable may not be set in the call to snp_issue_guest_request,
yet it is unconditionally written back to fw_err if fw_err is non-null.
This is undefined behavior, and currently returns uninitialized kernel
stack memory to user space.
Cc: Tom Lendacky <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Peter Gonda <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Dave Hansen <[email protected]>
Signed-off-by: Dionna Glaze <[email protected]>
---
drivers/virt/coco/sevguest/sevguest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c
index 112c0458cbda..7a62bfc063fc 100644
--- a/drivers/virt/coco/sevguest/sevguest.c
+++ b/drivers/virt/coco/sevguest/sevguest.c
@@ -307,7 +307,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 = 0;
u64 seqno;
int rc;
--
2.38.0.rc1.362.ged0d419d3c-goog
On Tue, Oct 11, 2022 at 6:23 PM Dionna Glaze <[email protected]> wrote:
>
> The err variable may not be set in the call to snp_issue_guest_request,
> yet it is unconditionally written back to fw_err if fw_err is non-null.
> This is undefined behavior, and currently returns uninitialized kernel
> stack memory to user space.
Should this be fixed in: snp_issue_guest_request()? Since other
callers might make similar mistakes.
And currently we have:
static long snp_guest_ioctl(...)
{
..
input.fw_err = 0xff;
..
}
Which I think is an attempt to make fw_err make sense if the FW is
never called, should we try to maintain that property?
>
> Cc: Tom Lendacky <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Peter Gonda <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Dave Hansen <[email protected]>
>
> Signed-off-by: Dionna Glaze <[email protected]>
> ---
> drivers/virt/coco/sevguest/sevguest.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virt/coco/sevguest/sevguest.c b/drivers/virt/coco/sevguest/sevguest.c
> index 112c0458cbda..7a62bfc063fc 100644
> --- a/drivers/virt/coco/sevguest/sevguest.c
> +++ b/drivers/virt/coco/sevguest/sevguest.c
> @@ -307,7 +307,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 = 0;
> u64 seqno;
> int rc;
>
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>
> And currently we have:
>
> static long snp_guest_ioctl(...)
> {
> ..
> input.fw_err = 0xff;
> ..
> }
>
> Which I think is an attempt to make fw_err make sense if the FW is
> never called, should we try to maintain that property?
fw_err = 0xff doesn't make sense to me actually. It's not a documented
code that the firmware was never called.
Still, we can simply pass fw_err to snp_issue_guest_request rather
than an unsigned long err, since a null pointer results in an -EINVAL.
--
-Dionna Glaze, PhD (she/her)
On Wed, Oct 19, 2022 at 12:58:12PM -0700, Dionna Amalie Glaze wrote:
> fw_err = 0xff doesn't make sense to me actually. It's not a documented
> code that the firmware was never called.
> Still, we can simply pass fw_err to snp_issue_guest_request rather
> than an unsigned long err, since a null pointer results in an -EINVAL.
Yes, pls do that. Such I/O function args are always a PITA anyway.
In retrospect, that handle_guest_request() with gazillion args should
have been made to take a struct as a single argument and populate it as
it operates.
The callers then would look at it and decide what to do.
Looking at the callers, they all take members of struct
snp_guest_request_ioctl and pass them in. A first step in cleaning that
up could be to simply pass that struct snp_guest_request_ioctl pointer
instead...
Oh well, in case folks feel bored. :-)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Oct 27, 2022 at 3:45 AM Borislav Petkov <[email protected]> wrote:
>
> On Wed, Oct 19, 2022 at 12:58:12PM -0700, Dionna Amalie Glaze wrote:
> > fw_err = 0xff doesn't make sense to me actually. It's not a documented
> > code that the firmware was never called.
> > Still, we can simply pass fw_err to snp_issue_guest_request rather
> > than an unsigned long err, since a null pointer results in an -EINVAL.
Yes this was not what my comments on the patch series intended.
On the host side we have `psp_ret = -1` inside of
__sev_platform_init_locked(). I think defining SEV_RET_NO_FW_CALL as
UINT32_MAX and moving to make all values of psp_ret where the psp is
not yet called would help callers understand errors more clearly.
>
> Yes, pls do that. Such I/O function args are always a PITA anyway.
>
> In retrospect, that handle_guest_request() with gazillion args should
> have been made to take a struct as a single argument and populate it as
> it operates.
>
> The callers then would look at it and decide what to do.
>
> Looking at the callers, they all take members of struct
> snp_guest_request_ioctl and pass them in. A first step in cleaning that
> up could be to simply pass that struct snp_guest_request_ioctl pointer
> instead...
>
> Oh well, in case folks feel bored. :-)
Ah, good idea. If I have some spare cycles.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette