Re: [PATCH v13 1/3] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 9/9/22 12:27, Kuppuswamy Sathyanarayanan wrote:
> +	u8 reserved[7] = {0};
...
> +	if (!req.reportdata || !req.tdreport || req.subtype ||
> +		req.rpd_len != TDX_REPORTDATA_LEN ||
> +		req.tdr_len != TDX_REPORT_LEN ||
> +		memcmp(req.reserved, reserved, 7))
> +		return -EINVAL;

Huh, so to look for 0's, you:

1. Declare an on-stack structure with a hard coded, magic numbered field
   that has to be zeroed.
2. memcmp() that structure
3. Feed memcmp() with another hard coded magic number

I've gotta ask: did you have any reservations writing this code?  Were
there any alarm bells going off saying that something might be wrong?

Using memcmp() itself is probably forgivable.  But, the two magic
numbers are pretty mortal sins in my book.  What's going to happen the
first moment someone wants to repurpose a reserved byte?  They're going
to do:

-	__u8 reserved[7];
+	__u8 my_new_byte;
+	__u8 reserved[6];

What's going to happen to the code you wrote?  Will it continue to work?
 Or will the memcmp() silently start doing crazy stuff as it overruns
the structure into garbage land?

What's wrong with:

	memchr_inv(&req.reserved, sizeof(req.reserved), 0)