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]
Date: Thu, 25 Jan 2024 12:59:52 +0100
From: Borislav Petkov <bp@...en8.de>
To: Nikunj A Dadhania <nikunj@....com>
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, dionnaglaze@...gle.com,
	pgonda@...gle.com, seanjc@...gle.com, pbonzini@...hat.com
Subject: Re: [PATCH v7 03/16] virt: sev-guest: Add SNP guest request structure

On Wed, Dec 20, 2023 at 08:43:45PM +0530, Nikunj A Dadhania wrote:
> -int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
> +int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_data *input,
> +			    struct snp_guest_request_ioctl *rio)
>  {
>  	struct ghcb_state state;
>  	struct es_em_ctxt ctxt;
>  	unsigned long flags;
>  	struct ghcb *ghcb;
> +	u64 exit_code;

Silly local vars. Just use req->exit_code everywhere instead.

>  	int ret;
>  
>  	rio->exitinfo2 = SEV_RET_NO_FW_CALL;
> +	if (!req)
> +		return -EINVAL;

Such tests are done under the variable which is assigned, not randomly.

Also, what's the point in testing req? Will that ever be NULL? What are
you actually protecting against here?

> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 469e10d9bf35..5cafbd1c42cb 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -27,8 +27,7 @@
>  
>  #include <asm/svm.h>
>  #include <asm/sev.h>
> -
> -#include "sev-guest.h"
> +#include <asm/sev-guest.h>
>  
>  #define DEVICE_NAME	"sev-guest"
>  
> @@ -169,7 +168,7 @@ static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
>  	return ctx;
>  }
>  
> -static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload, u32 sz)
> +static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_req *guest_req)

So we call the request everywhere "req". But you've called it
"guest_req" here because...

>  {
>  	struct snp_guest_msg *resp = &snp_dev->secret_response;
>  	struct snp_guest_msg *req = &snp_dev->secret_request;

.. there already is a "req" variable which is not a guest request thing
but a guest message. So why don't you call it "req_msg" instead and the
"resp" "resp_msg" so that it is clear what is what?

And then you can call the actual request var "req" and then the code
becomes more readable...

..

>  static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
>  {
>  	struct snp_report_req *req = &snp_dev->req.report;
> +	struct snp_guest_req guest_req = {0};

You have the same issue here.

If we aim at calling the local vars in every function the same, the code
becomes automatically much more readable.

And so on...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ