[PATCH] efi_loader: loosen buffer parameter check in efi_file_read

[PATCH] efi_loader: loosen buffer parameter check in efi_file_read

[Stefan Sørensen]

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@spectralink.com>
---
 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) {
+		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;
+
 	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)) {
 		ret = EFI_INVALID_PARAMETER;
 		goto error;
 	}
-- 
2.25.4

[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@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;
>  	}
>