Don't loop forever in load_certs() with buggy firmware

rmetrich commented 1 year ago

On DELL R350 booting DVD through RFS with BIOS 1.4.2 in Secure Boot, firmware returns EFI_BUFFER_TOO_SMALL but with new buffersize set to 0, which causes the load_certs() code to loop forever:

while (1) {
    efi_status = dir->Read(dir, &buffersize, buffer);
    if (efi_status == EFI_BUFFER_TOO_SMALL) {
        ...
        continue;
    }
    ...
}

This commit prevents such infinite loop. The new code doesn't check if new buffersize is 0 only, but if it's different from old one we passed, which may cover more buggy firmware cases at minor cost.

dennis-tseng99 commented 1 year ago

Just like your mention, the buffersize is 0 after read() which will result in ReallocatePool() return NULL, so maybe we can change the codes like the following to avoid forever loop and also do the sanity check for buffer got from ReallocatePool() :

while (1) {
        .....
    if (efi_status == EFI_BUFFER_TOO_SMALL) {
        buffer = ReallocatePool(buffer, old, buffersize);
            if (buffer == NULL) {
                perror(L"Failed to read directory %s - %r\n", PathName, EFI_OUT_OF_RESOURCES);
                goto done;
            }
        continue;
    } else if (EFI_ERROR(efi_status)) {
       .....
}

rmetrich commented 1 year ago

Yes I will squeeze everything once reviewed/accepted.

On Friday, January 27, 2023, Peter Jones @.***> wrote:

@vathpela commented on this pull request.

In shim.c:

efi_status = dir->Read(dir, &buffersize, buffer); if (efi_status == EFI_BUFFER_TOO_SMALL) {

buffer = ReallocatePool(buffer, old, buffersize);

continue;

if (buffersize != old) {

Can this be made to be one commit?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.< https://ci4.googleusercontent.com/proxy/LQW82rIz_ILPobRAA_ayNVTvTwEJvFAAgXSefYrA8WIIPIOgdzWbQGzPa6fhFatOuDzof6aO-Z78PxhvrJqz--7HhQGQwaWtnHA8Nca75AsOPg84KxtTFIsqPsvfwSoFkJpGaOl4gCafgcHWryoADfmbMu0wnTfnHj2ZIG66R6HTqE1kLUh7NOeFagFiyKvlNkkfm1-pTCiCA8D0pe_apkaDxnwTw-llpZaQVp9n2jEdjshSssAHFVzWWr3K=s0-d-e1-ft#https://github.com/notifications/beacon/AAI4C4ZWDZKAVQY3BXG6T6TWUQM7XA5CNFSM6AAAAAAT4MJX7GWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSL4USBE.gif>Message ID: @.***>

-- Renaud Sent from my phone, sorry for the typos...

rhboot / shim

Don't loop forever in load_certs() with buggy firmware #547