rhboot / shim

UEFI shim loader
Other
819 stars 289 forks source link

Fix typo in preserve_sbat_uefi_variable() #534

Closed Roo4L closed 1 year ago

Roo4L commented 1 year ago

During verification of current metadata curruption it was intended to verify whether component name matches SBAT_VAR_SIG, so sbat variable should at least has a length of SBAT_VAR_SIG, not SBAT_VAR_ORIGINAL. Current metadata version is verified later.

dennis-tseng99 commented 1 year ago

@Roo4L Thanks for your comment indeed. But, may I suggest we keep this line of code as its original one ? If you trace the codes next to the sbatsize < strlen(SBAT_VAR_ORIGINAL), you will see that more variable fields will be caught later, e.g. current_datestamp = nth_sbat_field(sbatc, sbatsize, 2); Hence, SBAT_VAR_ORIGINAL which covers the datestamp's length can guarantee the catching length is correct. Many thanks.

Roo4L commented 1 year ago

@dennis-tseng99 Hi! Thanks for your reply.

Well, as I see, if sbat variable is shorter that SBAT_VAR_ORIGINAL due to missing timestamp or version, it would anyway fail check-ups below and will be replaced, so it doesn't enhance the robustness. In my opinion, it's just confusing developers a little, but it is not so important for me. If you don't like the initiative, you may close the pull request.

jsetje commented 1 year ago

I suppose this could matter if we bump the metadata version and the new one has a more condensed time stamp or something like that.

(I hadn't seen this previously, I'm thinking about it over the weekend.)

jsetje commented 1 year ago

I do think we want to validate that the date/time/serial number exists there, which is what ORIGNAL does. If we ever changed that format, I would really want us to move to a new variable name rather than trying to find some way make it compatible.