Conversation
When this feature is enabled, the zero-byte check performed by skip_bytes is disabled. As a result, parsing will no longer fail when fields that are expected to be zero contain non-zero values. This enables forward compatibility when reserved fields are repurposed in new report versions. Users enabling this feature must take care to validate reserved fields manually if they rely on their contents for security or correctness. Signed-off-by: DGonzalezVillal <Diego.GonzalezVillalobos@amd.com>
cschramm
left a comment
There was a problem hiding this comment.
Thank you, @DGonzalezVillal. Your push to release any solution to unblock me is very kind.
This very minimal solution is perfectly fine for me.
Just bare in mind that the Encoder impls still assume that reserved bytes are zero, leading to invalid binary and broken features that rely on it, especially the cryptographic verification. #363 thus disables those parts while #366 reads and writes reserved areas to avoid that (leading to overhead even without the feature enabled).
I'd personally favor the #363 approach to avoid the footgun that comes with the feature otherwise, but that's up to you.
|
Hey @cschramm — I was looking more closely at your comment, and I think the On the decoding side, we already assume reserved bytes are zero and simply skip them. In both #363 This is also why @hyperfinitism has been pushing for a raw model that preserves all bytes and only interprets fields when needed. For that reason, I’m intentionally calling this quick implementation unsafe rather than lax. As you pointed out, it can lead to invalid binaries and broken behavior, and that risk fundamentally comes from how I do want to design a safer implementation that addresses what you and @hyperfinitism are asking for, but I don’t see a way to do that without significant changes to That said, I think your point is valid: if an unsafe (or “lax”) parser is enabled, then report verification should probably be disabled, since parsing and re-encoding the report no longer preserves integrity. Would that change make this a more acceptable interim solution? Thoughts, @hyperfinitism and @tylerfanelli? |
|
Any chance to get this (or a different short-term solution) merged? |
|
@tylerfanelli Are you ok with merging this or PR #363? I think after that we cut a minor release, because the more complete solution in #378 will have major changes. For the next major release we can also address the concerns in #369 thoughts?? |
When this feature is enabled, the zero-byte check performed by skip_bytes is disabled. As a result, parsing will no longer fail when fields that are expected to be zero contain non-zero values.
This enables forward compatibility when reserved fields are repurposed in newer report versions. Users enabling this feature must take care to validate reserved fields manually if they rely on their contents for security or correctness.
The approaches of preserving values in reserved fields or treating the report as an opaque blob both have potential. However, based on concerns raised by @cschramm and @hyperfinitism, I think there is a larger design discussion to be had—one that could ultimately change how some of these structs are built and may require a breaking API release. To avoid dependency issues like the one @cshramm pointed out in #370
, I’m hesitant to put such changes behind a feature flag. The alternative—introducing a separate report type—is also not ideal.
I don’t want to block @cshramm any longer, so this change immediately addresses the forward compatibility problem. I’m aware this approach has trade-offs, such as losing access to the parsed bytes of invalid reserved fields. However, since we currently have no appropriate fields to store that data, the report maintains type safety by only exposing known, validated fields. Additionally, the API already returns the raw binary report, so those bytes can still be inspected if needed.
If there are additional functions or features you think would make this more immediately usable, please let me know. I’ll start a separate discussion to explore how we might incorporate @hyperfinitism’s ideas into a future major release.