lkml.org 
[lkml]   [2022]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v13 1/3] x86/tdx: Add TDX Guest attestation interface driver
From
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)

\
 
 \ /
  Last update: 2022-09-09 21:47    [W:0.726 / U:0.584 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site