Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Buffer Too Small" when loading initrd from ext2 partition #38

Closed
YorikSar opened this issue Aug 19, 2022 · 9 comments
Closed

"Buffer Too Small" when loading initrd from ext2 partition #38

YorikSar opened this issue Aug 19, 2022 · 9 comments

Comments

@YorikSar
Copy link

While trying to make systemd-boot use these drivers to load initrd and kernel from custom XBOOTLDR partition, I get "Buffer Too Small" error when I try using ext2 with efifs driver instead of vfat for it. Is there some trick I'm missing for using these drivers?

You can reproduce my error by running

docker run --device /dev/kvm --rm -it yoriksar/nixos-qemu-vm:3ifn9pacv6wqv1r49j002p01cz7w4ama

It's a NixOS system wrapped in QEMU VM in Docker image. Note that similar system that uses FAT boots just fine, even though it also loads ext2_64.efi driver:

docker run --device /dev/kvm --rm -it yoriksar/nixos-qemu-vm:7nbbvwpjwa292ws2k58vr98vmlq7pgvc

(you may omit --device /dev/kvm if you don't want containers to get access to it, but it will be slow)

Both examples use the same version of everything, only the format of the boot drive differs.

In my previous attempts I managed to get more debug from systemd-boot:

OpenVolume
OpenVolume
Open(3E7F2998 <ROOT>, "\EFI\nixos\775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi")
  RET: 3EE78E18
Close(3E7F2998|'/') <ROOT>
GetInfo(3EE78E18|'/EFI/nixos/775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi', 0)
Get regular file information
GetInfo(3EE78E18|'/EFI/nixos/775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi', 600)
Get regular file information
Read(3EE78E18|'/EFI/nixos/775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi', 7821408)
Close(3EE78E18|'/EFI/nixos/775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi')
OpenVolume
Open(3E7F2998 <ROOT>, "EFI\nixos\688xmj1rc311djm1ql9c80kr4gmcc3va-initrd-linux-5.15.55-initrd.efi")
  RET: 3E7DB018
GetInfo(3E7DB018|'/EFI/nixos/688xmj1rc311djm1ql9c80kr4gmcc3va-initrd-linux-5.15.55-initrd.efi', 592)
Get regular file information
EFI stub: ERROR: Failed to get file info
Close(3E7DB018|'/EFI/nixos/688xmj1rc311djm1ql9c80kr4gmcc3va-initrd-linux-5.15.55-initrd.efi')
Close(3E7F2998|'/') <ROOT>
EFI stub: ERROR: Failed to load initrd!
EFI stub: ERROR: efi_main() failed!
Close(3E7F2998|'/') <ROOT>
Failed to execute NixOS (\EFI\nixos\775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi): Buffer Too Small

it seems like it fails to get file information using this driver first, so "Buffer Too Small" might not be the root issue.

@pbatard
Copy link
Owner

pbatard commented Sep 3, 2022

I suggest you recompile EfiFs and add extra debug information in file.cFileGetInfo().

From what I can see, the kernel (775xip76m7ipmad91aqg4qmzv11ag5f7-linux-5.15.55-bzImage.efi) is properly queried for size and it's 7821408 bytes are properly read and the problem happens with the GetInfo() call to get the initrd size, which appears to return EFI_BUFFER_TOO_SMALL, I guess because the buffer being passed to it smaller than the minimum size we require (which is sizeof(EFI_FILE_INFO) + MAX_PATH * sizeof(CHAR16)).

Per the UEFI Specs (Section 13.5 File Protocol), an application calling GetInfo() should expect that EFI_BUFFER_TOO_SMALL may be returned by the file system driver, and should have provision to adjust its buffer size and try the call again, but I'm guessing whatever EFI bootloader you are using does not do that and is instead assuming that their buffer is always large enough for the call.

Note that, because we always do request a buffer that can accommodate a file size of MAX_PATH which one may see as overreaching, nowhere in the UEFI specs is it implied that the buffer size for GetInfo() is not going to be larger than the exact size needed (and as a matter of fact one could argue that, on repeated calls like directory listing, it is better to request a size that is much larger than the exact size needed to avoid going through repeated buffer reallocation every time a file is found that has a longer name than the maximum previous one).

So my guess is that whoever wrote your bootloader may have taken a shortcut and allocated a buffer for GetInfo() that they thought was more than large enough to accommodate the initrd file name, and therefore that they could skip the buffer reallocation dance for initrd (whereas they don't seem to do that for the kernel, which is why it doesn't fail there).

For the record, here's what I believe happens:

  1. GetInfo() with buffer size 0 is called for the kernel
  2. EfiFs GetInfo() returns that it needs a buffer size of 600 to accommodate the request (which is the value of sizeof(EFI_FILE_INFO) + MAX_PATH * sizeof(CHAR16) in bytes and what EfiFs always requests as a minimum buffer size for GetInfo())
  3. Buffer is allocated by the bootloader and GetInfo() with buffer size 600 is called for the kernel, whereupon EfiFs returns the expected GetInfo() data
  4. GetInfo() with buffer size 592 is called for initrd
  5. Just like it did for the previous request, EfiFs GetInfo() returns that it needs a buffer size of 600 to accommodate the request
  6. Your bootloader assumed that "592 bytes should be large enough for everybody" and bails out instead of reallocating a buffer of the requested size and retrying the GetInfo() call.

@YorikSar
Copy link
Author

YorikSar commented Sep 3, 2022

Thanks a lot for your time and suggestions.

The boot loader in question is systemd-boot, which just recently got the ability to load EFI drivers. The only reference to GetInfo() that I see is here: https://github.com/systemd/systemd-stable/blob/v251.3/src/boot/efi/util.c#L509-L515 - and it seems like it is handled properly.

I will try adding some debug info where you've suggested next week, thanks for the pointer.

@pbatard
Copy link
Owner

pbatard commented Sep 3, 2022

Okay.

The other possibility is that 592 and 600 are both larger than the size we need for GetInfo(), which should be around 592, so the GetInfo(3E7DB018|'/EFI/nixos/688xmj1rc311djm1ql9c80kr4gmcc3va-initrd-linux-5.15.55-initrd.efi', 592) call does not actually bail out on the initial data buffer size check but later (you may want to instrument that we are indeed not returning on the initial size check by adding a print statement there).

So the other place that will return a buffer too small error is here, which would imply that the EfiFs call to Utf8ToUtf16NoAllocUpdateLen() failed with EFI_BUFFER_TOO_SMALL, but I don't really see how that can happen (you may want to instrument this as well).

The fact that we get EFI stub: ERROR: Failed to get file info right after Get regular file information seems to hint that the EfiFs call to GetInfo() is returning EFI_BUFFER_TOO_SMALL.

What makes no sense however is that the systemd code does have a proper retry for EFI_BUFFER_TOO_SMALL, which means that, if GetInfo() really returned EFI_BUFFER_TOO_SMALL, we should see a second set of

GetInfo(3EE78E18|'/EFI/nixos/688xmj1rc311djm1ql9c80kr4gmcc3va-initrd-linux-5.15.55-initrd.efi', ???)
Get regular file information

from your log, which we don't...

Considering that it doesn't come from the systemd code you pointed to, I'm starting to think that the EFI stub: ERROR: Failed to get file info line comes from a different section of code.

Most likely, the problem is located in the Linux kernel EFI Stub and how it handles GetInfo()...

@pbatard
Copy link
Owner

pbatard commented Sep 3, 2022

Yup, here's your issue, and it's a pure kernel one:

  1. The kernel defines a struct finfo for GetInfo() of a very fixed size:
#define MAX_FILENAME_SIZE	256
(...)
struct finfo {
	efi_file_info_t info;
	efi_char16_t	filename[MAX_FILENAME_SIZE];
};
  1. Then the kernel uses that fixed structure for GetInfo() query with no provision whatsoever for the possibility that its buffer might be too small:
	info_sz = sizeof(struct finfo);
	status = fh->get_info(fh, &info_guid, &info_sz, fi);
	if (status != EFI_SUCCESS) {
		efi_err("Failed to get file info\n");
		fh->close(fh);
		return status;
	}

And whereas EfiFs officially uses the same MAX_FILENAME_SIZE as the kernel (which we call MAX_PATH, but which we do set to 256), the part that triggers the EFI_BUFFER_TOO_SMALL is that we add 2 bytes to the size of our FILE_INFO structure on account that we use a 1-sized array for CHAR16 FileName[1];.

So the size of the buffer we request is 2 bytes larger than the fixed size buffer the kernel can accommodate, and thus the kernel bails out...

@pbatard
Copy link
Owner

pbatard commented Sep 3, 2022

Now, one way I could "fix" this in EfiFs is reduce the size of MAX_PATH so that it falls within the kernel expected limit. And as a matter of fact, I think EfiFs should use the SIZE_OF_EFI_FILE_INFO macro that is defined for EFI applications instead of sizeof(EFI_FILE_INFO) (which will add two bytes on account of the CHAR16 FileName[1]; buffer at the end of the struct) because, whereas using a size with 2 extra bytes is convenient to make sure we always have space for the NUL terminator, it'll probably lead to issues and confusion down the line.

However, I think the kernel should really fix their GetInfo() retrieval, especially as UEFI makes not guarantee about MAX_FILENAME_SIZE being 512 bytes or less. As a matter of fact, you can find direct example of it being larger than that here, which means that, at the very least, the kernel should increase their constant or, better, perform a GetInfo() that handles EFI_BUFFER_TOO_SMALL...

@YorikSar
Copy link
Author

YorikSar commented Sep 3, 2022

Thanks! This is awesome research. I will try to apply suggested fixes to both kernel and efifs and see what helps.
I think it's better to address this in both EfiFs for compatibility with old Linux kernels and kernel for compatibility with any other drivers that go beyond defaults, but stay within UEFI spec.

@pbatard
Copy link
Owner

pbatard commented Sep 5, 2022

I'm going to push v1.9 of EfiFs which should sort the EFI Stub issue (by making sure that our buffer is the same size as the fixed size the kernel uses). This will close this issue as a result, but feel free to report if you find there's still an issue.

@pbatard pbatard closed this as completed in d31ec7b Sep 5, 2022
@YorikSar
Copy link
Author

YorikSar commented Sep 6, 2022

@pbatard Thanks a lot! I've dropped v1.9 in place of v1.8 and it just works now. You're awesome!

@pbatard
Copy link
Owner

pbatard commented Sep 6, 2022

Excellent. Thanks for reporting back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants