rhboot / shim

UEFI shim loader
Other
856 stars 292 forks source link

Freshly compiled Shim-15.7 (x86_64) cannot load grubx64.efi #624

Open Jurij-Ivastsuk opened 9 months ago

Jurij-Ivastsuk commented 9 months ago

Freshly compiled Shim-15.7 (x86_64) cannot load grubx64.efi, only after "falling back to default loader" grubx64.efi can be loaded. Does anyone know the cause? How can this problem be solved?

IMG_5120

Thanks in advance! Jurij

Jurij-Ivastsuk commented 9 months ago

Here is some additional information: I have checked shim.c and the following is the result: in function: shim_init(void) in function: EFI_STATUS set_second_stage (EFI_HANDLE image_handle) second_stage is set to "\grubx64.efi" but in function: efi_status = init_grub(image_handle); EFI_STATUS init_grub(EFI_HANDLE image_handle) second_stage - no longer has a value !!! The value "\grubx64.efi" has been lost... second_stage is defined in: load-options.c CHAR16 *second_stage; Compiled with gcc-10 (10.2.0-19), g++-10 (10.2.0-19), gnu-efi (3.0.9-2)

Therefore, I believe that this is the cause of this problem. But why does second_stage lose its value? The value "\grubx64.efi" is set earlier, before the execution of init_grub()...

Jurij-Ivastsuk commented 9 months ago

The solution was found. Please have a look here: https://github.com/rhboot/shim/pull/625

AndrewSav commented 9 months ago

@Jurij-Ivastsuk did you manage to understand what is replacing second_stage or why it is being replaced? From the comment on your PR it looks like an interaction with some external entity is assumed which is writing the value of this variable and the shim does not write this value itself. I'm curious what that external entity would be?

Jurij-Ivastsuk commented 9 months ago

@AndrewSav My PR 625 was rejected, and that is correct. The first attempt was pretty quick but not a good solution for everyone. But pointing out the weakness of this solution helped me move forward. I think I have actually localized the problem. I still need to do a lot of testing to be able to say that this would actually be a solution for everyone.

Jurij-Ivastsuk commented 9 months ago

@AndrewSav We have localized the problem. The overwriting of the value for second_stage takes place in the function parse_load_options() in the file load-options.c Because the function split_load_options() returns a non-printable ascii character, the originally set value \grubx64.efi is overwritten by this character. You can view the problem description and the solution here: [https://github.com/rhboot/shim/pull/626] I hope that this time my pull request will not be rejected.

Jurij-Ivastsuk commented 9 months ago

After some research I found a similar solution. It starts at exactly the same point as I did and checks whether the value of this variable represents a valid path before overwriting the variable "second_stage": https://github.com/rhboot/shim/commit/36adff84a8251f0593e2c524268116c05688170b

Various checks are implemented there, but the check whether the first character contains a non-printable ASCII character is omitted. If you are looking for a general solution to this problem, perhaps a combination of both solutions: mine and the one just mentioned would be a better solution that takes several eventualities into account. But in my opinion, one should make a fundamental consideration whether the function parse_load_options() has any justification at all.

mikebeaton commented 9 months ago

It's usually not good practice to post comments twice - as you're ofc aware, your issue is discussed at both places you've posted, and probably other people reading this who are more or less up to speed on the issue are aware of that too. (So basically, people who care much get your message twice - and have to scroll through it twice in future, if scrolling through both threads - and only people who don't care much see it maybe once or never!) Thx. (Feel free to just delete one, if on reflection you feel this comment is justified - and I'll delete this! :-) )

Jurij-Ivastsuk commented 9 months ago

@mikebeaton Thanks for the tip. According to my consideration, there are different readers: some who only participate in discussions and others like the reviewers who need more basic information to judge whether the proposed solution is good or not. To inform both groups I posted the information on both channels. Only in the latter case the text is identical.

AndrewSav commented 9 months ago

It does not annoy me personally, if it is posted twice. I can skip if I have read it before. I agree that the audiences might not cross over in parts. As long as it is not every single message duplicated it's fine IMO.

ghost commented 8 months ago

Of course it got rejected. Because people are completely stupid.

Jurij-Ivastsuk commented 8 months ago

@PC-Doctor666 Hello, sorry, but your comments are unfortunately not very constructive.