ext_config: Fixes config only operations.

[larrydewey]

Previously we were passing the u64 value of the pointer to the vector of bytes the extended report certificates were being loaded from. However, when handling the case of an extended configuration only, because the pointer value of an empty vector is not 0, this was causing an error in the kernel when attempting to access the "pointer" being provided. By modifying the underlying type of bytes to Option<Vec<u8>>, it allows the default value to be set to None, and conditionally update the extended config certs_address field when certificate bytes are present.

[DGonzalezVillal]

Clever, so when passing the value as none instead of a ptr, then the firmware knows it's supposed to be empty?

[tylerfanelli]

If the certificate input is empty, wouldn't the ext_config certs_len be set to 0? Why would the buffer be read in that case?

[tylerfanelli]

See above comment.

[larrydewey]

If the certificate input is empty, wouldn't the ext_config certs_len be set to 0? Why would the buffer be read in that case?

It isn't that it is being read from the user's input, but that we created the bytes vector to ensure the data lived long enough before being dropped. In this case, because the vector was originally vec![] the pointer was actually 0x1, which caused an issue with the kernel. By setting the value to None by default, and wrapping the condition with an if let Some(), we can check if the bytes array has been populated after converting the Rust CertTable to the C-CertTable structure.

[tylerfanelli]

If the certificate input is empty, wouldn't the ext_config certs_len be set to 0? Why would the buffer be read in that case?

It isn't that it is being read from the user's input, but that we created the bytes vector to ensure the data lived long enough before being dropped. In this case, because the vector was originally vec![] the pointer was actually 0x1, which caused an issue with the kernel. By setting the value to None by default, and wrapping the condition with an if let Some(), we can check if the bytes array has been populated after converting the Rust CertTable to the C-CertTable structure.

I'm still not seeing how the kernel gets tripped up here unless a read takes place. If the vec![] is of size 0 (i.e. there are no certs) then the buffer would never be read.

[DGonzalezVillal]

lgtm

[larrydewey]

If the certificate input is empty, wouldn't the ext_config certs_len be set to 0? Why would the buffer be read in that case?

It isn't that it is being read from the user's input, but that we created the bytes vector to ensure the data lived long enough before being dropped. In this case, because the vector was originally vec![] the pointer was actually 0x1, which caused an issue with the kernel. By setting the value to None by default, and wrapping the condition with an if let Some(), we can check if the bytes array has been populated after converting the Rust CertTable to the C-CertTable structure.

I'm still not seeing how the kernel gets tripped up here unless a read takes place. If the vec![] is of size 0 (i.e. there are no certs) then the buffer would never be read.

The Kernel gets tripped up because input.certs_address is not 0x0, it is 0x1, which means it enters the conditional block from the link you provided above. When this happens, !input.certs_len becomes true, and returns -EINVAL. In the situation where we want to set the REPORTED_TCB via config_only, we don't want this to cause the kernel to puke an -EINVAL.

Updating the pointer to the certs_address only when the certificate bytes are present fixes this problem.

Further, there is a use after free issue which is happening because of the if-let block. I recently learned that when using if-let, the value in Some(value) is a copy, and drops out of scope at the end of the if-let block. Using Some(ref value), we are specifying we want to refer to the original value instead of copying it.

I have tested setting and resetting the REPORTED_TCB on a machine with these patches, and it is working as expected now.

[tylerfanelli]

I see, so when we are only setting the extended config, we have an empty certs buffer (i.e. certs_address != 0 and certs_len == 0), and thus it fails. What we want to do is pass certs_address == 0 (which is basically what we are doing with a None value being passed).

[tylerfanelli]

LGTM.