[PATCH] efi_loader: loosen buffer parameter check in efi_file_read

[Heinrich Schuchardt]

On 21.07.20 16:02, Stefan Sørensen wrote:
> When reading a directory, EFI_BUFFER_TOO_SMALL should be returned when
> the supplied buffer is too small, so a use-case is to call
> efi_file_read with *buffer_size=0 and buffer=NULL to obtain the needed
> size before doing the actual read.
>
> So drop the buffer!=NULL requirement when doing directory reads, but
> keep them when doing file reads.
>
> This fix allows the Redhat shim fallback to run and e.g. Fedora 32 now
> boots out of the box.
>
> Signed-off-by: Stefan Sørensen <stefan.sorensen at spectralink.com>

Thanks a lot for reporting the issue.

The coding looks functionally ok but I think there is some room for
simplification and relaxation of the test condition in dir_read().

> ---
>  lib/efi_loader/efi_file.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 19afa69f53..7b0dcbdba9 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -349,6 +349,11 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
>  	efi_status_t ret;
>  	loff_t file_size;
>
> +	if (!buffer_size || !buffer) {

buffer_size is already tested by the caller. No need to check it again.

> +		ret = EFI_INVALID_PARAMETER;
> +		return ret;
> +	}
> +
>  	ret = efi_get_file_size(fh, &file_size);
>  	if (ret != EFI_SUCCESS)
>  		return ret;
> @@ -377,6 +382,9 @@ static efi_status_t dir_read(struct file_handle *fh, u64 *buffer_size,
>  	u64 required_size;
>  	u16 *dst;
>
> +	if (!buffer_size || (*buffer_size && !buffer))
> +		return EFI_INVALID_PARAMETER;
> +

As long as buffer_size is too small we should return EFI_BUFFER_TOO_SMALL.

It is enough to check the buffer address after verifying that the
buffer_size is ok.

>  	if (set_blk_dev(fh))
>  		return EFI_DEVICE_ERROR;
>
> @@ -443,7 +451,7 @@ static efi_status_t EFIAPI efi_file_read(struct efi_file_handle *file,
>
>  	EFI_ENTRY("%p, %p, %p", file, buffer_size, buffer);
>
> -	if (!buffer_size || !buffer) {
> +	if (!buffer_size || (*buffer_size && !buffer)) {

No need to check buffer here. We check it in the callee.

Best regards

Heinrich

>  		ret = EFI_INVALID_PARAMETER;
>  		goto error;
>  	}
>