efi_loader: loosen buffer parameter check in efi_file_read_int

Message ID 20210428081512.25890-1-peng.fan@oss.nxp.com
State Superseded
Headers show
Series
  • efi_loader: loosen buffer parameter check in efi_file_read_int
Related show

Commit Message

Peng Fan (OSS) April 28, 2021, 8:15 a.m.
From: Peng Fan <peng.fan@nxp.com>


This is same issue as https://bugzilla.redhat.com/show_bug.cgi?id=1733817,
but that fix was wrongly partial reverted.

To Fedora shim loader, when buffer is NULL, 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.

Otherwise, we always met "Could not read \EFI\: Invalid Parameter"

Fixes: db12f518edb0("efi_loader: implement non-blocking file services")
Signed-off-by: Peng Fan <peng.fan@nxp.com>

Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Stefan Sørensen <stefan.sorensen@spectralink.com>
---
 lib/efi_loader/efi_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.0

Comments

Peter Robinson April 28, 2021, 12:22 p.m. | #1
On Wed, Apr 28, 2021 at 8:43 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>

> From: Peng Fan <peng.fan@nxp.com>

>

> This is same issue as https://bugzilla.redhat.com/show_bug.cgi?id=1733817,

> but that fix was wrongly partial reverted.

>

> To Fedora shim loader, when buffer is NULL, 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.

>

> Otherwise, we always met "Could not read \EFI\: Invalid Parameter"

>

> Fixes: db12f518edb0("efi_loader: implement non-blocking file services")

> Signed-off-by: Peng Fan <peng.fan@nxp.com>

> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

> Cc: Stefan Sørensen <stefan.sorensen@spectralink.com>


Tested-by: Peter Robinson <pbrobinson@gmail.com>


Tested on a patched 2021.04 with Fedora 34. Thanks!

> ---

>  lib/efi_loader/efi_file.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c

> index 204105e25a..6b3f5962be 100644

> --- a/lib/efi_loader/efi_file.c

> +++ b/lib/efi_loader/efi_file.c

> @@ -554,7 +554,7 @@ static efi_status_t efi_file_read_int(struct efi_file_handle *this,

>         efi_status_t ret = EFI_SUCCESS;

>         u64 bs;

>

> -       if (!this || !buffer_size || !buffer)

> +       if (!this || !buffer_size)

>                 return EFI_INVALID_PARAMETER;

>

>         bs = *buffer_size;

> --

> 2.30.0

>
Heinrich Schuchardt April 28, 2021, 12:49 p.m. | #2
On 28.04.21 10:15, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>

>

> This is same issue as https://bugzilla.redhat.com/show_bug.cgi?id=1733817,

> but that fix was wrongly partial reverted.

>

> To Fedora shim loader, when buffer is NULL, 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.

>

> Otherwise, we always met "Could not read \EFI\: Invalid Parameter"


The EFI specification does not require this behavior. EDK II does not
implement it either. See for instance SemihostFileRead(),
FvSimpleFileSystemRead().

To determine the file size you have to use GetInfo(). Please, fix the
client code.

Best regards

Heinrich

>

> Fixes: db12f518edb0("efi_loader: implement non-blocking file services")

> Signed-off-by: Peng Fan <peng.fan@nxp.com>

> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

> Cc: Stefan Sørensen <stefan.sorensen@spectralink.com>

> ---

>  lib/efi_loader/efi_file.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c

> index 204105e25a..6b3f5962be 100644

> --- a/lib/efi_loader/efi_file.c

> +++ b/lib/efi_loader/efi_file.c

> @@ -554,7 +554,7 @@ static efi_status_t efi_file_read_int(struct efi_file_handle *this,

>  	efi_status_t ret = EFI_SUCCESS;

>  	u64 bs;

>

> -	if (!this || !buffer_size || !buffer)

> +	if (!this || !buffer_size)

>  		return EFI_INVALID_PARAMETER;

>

>  	bs = *buffer_size;

>
Heinrich Schuchardt April 28, 2021, 1:02 p.m. | #3
On 28.04.21 14:49, Heinrich Schuchardt wrote:
> On 28.04.21 10:15, Peng Fan (OSS) wrote:

>> From: Peng Fan <peng.fan@nxp.com>

>>

>> This is same issue as https://bugzilla.redhat.com/show_bug.cgi?id=1733817,

>> but that fix was wrongly partial reverted.

>>

>> To Fedora shim loader, when buffer is NULL, a use-case is to call

>> efi_file_read with *buffer_size=0 and buffer=NULL to obtain the needed


An EFI binary cannot directly call efi_file_read(). It calls
EFI_FILE_PROTOCOL.Read().

>> size before doing the actual read.

>>

>> Otherwise, we always met "Could not read \EFI\: Invalid Parameter"


Please, describe the problem in UEFI terms.

>

> The EFI specification does not require this behavior. EDK II does not

> implement it either. See for instance SemihostFileRead(),

> FvSimpleFileSystemRead().

>

> To determine the file size you have to use GetInfo(). Please, fix the

> client code.

>

> Best regards

>

> Heinrich


Please, make it clear in the commit message, please, that this patch is
about reading a *directory entry* and not about reading a file.

Best regards

Heinrich

>

>>

>> Fixes: db12f518edb0("efi_loader: implement non-blocking file services")

>> Signed-off-by: Peng Fan <peng.fan@nxp.com>

>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

>> Cc: Stefan Sørensen <stefan.sorensen@spectralink.com>

>> ---

>>  lib/efi_loader/efi_file.c | 2 +-

>>  1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c

>> index 204105e25a..6b3f5962be 100644

>> --- a/lib/efi_loader/efi_file.c

>> +++ b/lib/efi_loader/efi_file.c

>> @@ -554,7 +554,7 @@ static efi_status_t efi_file_read_int(struct efi_file_handle *this,

>>  	efi_status_t ret = EFI_SUCCESS;

>>  	u64 bs;

>>

>> -	if (!this || !buffer_size || !buffer)

>> +	if (!this || !buffer_size)

>>  		return EFI_INVALID_PARAMETER;

>>

>>  	bs = *buffer_size;

>>

>

Patch

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 204105e25a..6b3f5962be 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -554,7 +554,7 @@  static efi_status_t efi_file_read_int(struct efi_file_handle *this,
 	efi_status_t ret = EFI_SUCCESS;
 	u64 bs;
 
-	if (!this || !buffer_size || !buffer)
+	if (!this || !buffer_size)
 		return EFI_INVALID_PARAMETER;
 
 	bs = *buffer_size;