| Message ID | 20220909192708.1113126-1-sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| Headers | show |
| Series | Add TDX Guest Attestation support | expand |
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)
On 9/9/22 12:41 PM, Dave Hansen wrote: > 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) I did not consider the hard coding issue. It is a mistake. Your suggestion looks better. I will use it.