[U-Boot] [RFC 1/1] efi_loader: check parameters of efi_file_open()

[AKASHI, Takahiro]

On Sun, Jul 22, 2018 at 01:34:13PM +0200, Heinrich Schuchardt wrote:
> Check the parameters of efi_file_open().
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> Hello Takahiro,
> 
> this patch is necessary to become more UEFI compliant. But it interferes
> with your FAT patch series.
> 
> You might integrate the changes into the next version of you patch series.
> Otherwise I keep the patch until your series is merged.

I still believe, as I mentioned elsewhere, that such kind of patches
be compiled into a separate (follow-up) patch series.

Speaking of your specific patch, it looks good as a whole, please let me
advise you that a section number in UEFI specification or UEFI SCT
test case number be included in your comment wherever possible which
would make it easier for not only me but also others to assure your
changes.

> Best regards
> 
> Heinrich
> ---
>  lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 07696a8f56..8f04943670 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -202,15 +202,37 @@ static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file,
>  		s16 *file_name, u64 open_mode, u64 attributes)
>  {
>  	struct file_handle *fh = to_fh(file);
> +	efi_status_t ret;
>  
>  	EFI_ENTRY("%p, %p, \"%ls\", %llx, %llu", file, new_handle, file_name,
>  		  open_mode, attributes);
>  
> -	*new_handle = file_open(fh->fs, fh, file_name, open_mode);
> -	if (!*new_handle)
> -		return EFI_EXIT(EFI_NOT_FOUND);
> +	/* Check parameters */
> +	if (!file || !file || !file_name) {

nit: duplicated !file

> +		ret = EFI_INVALID_PARAMETER;

Strangely, EFI_INVALID_PARAMETER is not listed in "Status Codes Returned"
at section 13.5 File Protocol, p.524.

> +		goto out;
> +	}
> +	if (open_mode != EFI_FILE_MODE_READ &&
> +	    open_mode != (EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE) &&
> +	    open_mode != (EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE |
> +			 EFI_FILE_MODE_CREATE)) {

(open_mode | CREATE) && !(open_mode | WRITE) would be simpler.

> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	if ((!(open_mode & EFI_FILE_MODE_CREATE) && attributes) ||

OK,

> +	    (attributes & (EFI_FILE_READ_ONLY | ~EFI_FILE_VALID_ATTR))) {

Why is EFI_FILE_READ_ONLY not allowed?

> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	/* Open file */
> +	*new_handle = file_open(fh->fs, fh, file_name, open_mode);
> +	if (*new_handle)
> +		ret = EFI_SUCCESS;
> +	else
> +		ret = EFI_NOT_FOUND;

Should we always return NOT_FOUND here?
(No choices, probably)

-Takahiro AKASHI

> +out:
> +	return EFI_EXIT(ret);
>  }
>  
>  static efi_status_t file_close(struct file_handle *fh)
> -- 
> 2.18.0
>