kernel/attest: validate AttestServicesOp and AttestSingleServiceOp#711
kernel/attest: validate AttestServicesOp and AttestSingleServiceOp#711stefano-garzarella wants to merge 1 commit intococonut-svsm:mainfrom
Conversation
Commit 0ae2ab7 ("Add attest_multiple_services") used `read_from_guest` to create an instance of `AttestServicesOp` or `AttestSingleServiceOp`, but we didn't validate them like we do in `try_from_as_ref()`. So let's provide for both a new `try_from_guest()` method where we can validate them. Probably in the future we should provide a trait (e.g. `FromGuest`) that each structure can implement in order to standardize our code. Fixes: 0ae2ab7 ("Add attest_multiple_services") Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
| if !ops.is_reserved_clear() { | ||
| return Err(SvsmReqError::invalid_parameter()); | ||
| } |
There was a problem hiding this comment.
Do we not also want to check the validity requirements of the nonce and manifest?
We can then say that there's the PreAttestServicesOp type that is structurally all the fields to read, and try_from_guest returns AttestServicesOp which is ensured to have valid fields. That allows all the get_ methods to not need to do any wellformedness checks.
There was a problem hiding this comment.
Do we not also want to check the validity requirements of the nonce and manifest?
Maybe yes, but I just followed what we did in try_from_as_ref() so I guess we should fix both.
We can then say that there's the PreAttestServicesOp type that is structurally all the fields to read, and try_from_guest returns AttestServicesOp which is ensured to have valid fields. That allows all the get_ methods to not need to do any wellformedness checks.
Yep, this is something similar to what I did in #603 where I added a generic validate() method called by the try_from_* API. Maybe I can do the same here (in another commit). WDYT?
There was a problem hiding this comment.
I think even moreso here and there. both TpmSendCommandRequest and AttestServicesOp ought to have their fields in TpmSendCommandRequestState/AttestServicesOpState structs that get embedded in TpmSendCommandRequest/AttestServicesOp only if their values pass validation. This ensures that operations are only associated with the struct type that maintains the wellformedness invariant.
There was a problem hiding this comment.
If we make fields private and provide only try_from* method to build those structures, we should have the same, without embending them, no?
Or maybe I'm missing something.
There was a problem hiding this comment.
If we make fields private and provide only
try_from*method to build those structures, we should have the same, without embending them, no? Or maybe I'm missing something.
@deeglaze WDYT?
stefano-garzarella
added
the
wait-for-update
3 participants
Commit 0ae2ab7 ("Add attest_multiple_services") used
read_from_guestto create an instance ofAttestServicesOporAttestSingleServiceOp, but we didn't validate them like we do intry_from_as_ref().So let's provide for both a new
try_from_guest()method where we can validate them. Probably in the future we should provide a trait (e.g.FromGuest) that each structure can implement in order to standardize our code.Fixes: 0ae2ab7 ("Add attest_multiple_services")