efi_loader: check parameters of efi_file_open()
RFC 1/1 efi_loader: check parameters of efi_file_open()
Check the parameters of efi_file_open().
---
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.
Best regards
Heinrich
--- lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)
lib/efi_loader/efi_file.c +26 -4
@@ -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) { + ret =EFI_INVALID_PARAMETER ; + 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 )) { + ret =EFI_INVALID_PARAMETER ; + goto out; + } +if ((!(open_mode &EFI_FILE_MODE_CREATE ) && attributes) || + (attributes & (EFI_FILE_READ_ONLY | ~EFI_FILE_VALID_ATTR ))) { + 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 ; +out: +return EFI_EXIT (ret); }static efi_status_t file_close (struct file_handle *fh)
Signature · 1 line
2.18.0
AKASHI
RFC 1/1 efi_loader: check parameters of efi_file_open()
Quoted 11 lines · Heinrich Schuchardt
Check the parameters of
efi_file_open().Signed-off-by Heinrich Schuchardt <[email protected]>---
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.
Quoted 15 lines
Best regards
Heinrich
--- lib/efi_loader/efi_file.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)lib/efi_loader/efi_file.c +0
-0@@ -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);
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 211
Quoted 10 lines
lib/efi_loader/efi_file.c +3
-3+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
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 212
Quoted 1 line
lib/efi_loader/efi_file.c +1
-0+ ret =EFI_INVALID_PARAMETER ;
Strangely, EFI_INVALID_PARAMETER is not listed in "Status Codes Returned" at section 13.5 File Protocol, p.524.
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 218
Quoted 6 lines
lib/efi_loader/efi_file.c +6
-0+ 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.
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 222
Quoted 4 lines
lib/efi_loader/efi_file.c +4
-0+ ret =EFI_INVALID_PARAMETER ; + goto out; + } + if ((!(open_mode & EFI_FILE_MODE_CREATE) && attributes) ||
OK,
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 223
Quoted 1 line
lib/efi_loader/efi_file.c +1
-0+ (attributes & (EFI_FILE_READ_ONLY | ~EFI_FILE_VALID_ATTR))) {
Why is EFI_FILE_READ_ONLY not allowed?
lib/efi_loader/efi_file.c static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file, · line 233
Quoted 11 lines
lib/efi_loader/efi_file.c +9
-1+ 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
Quoted 8 lines
lib/efi_loader/efi_file.c +2
-0+out: +return EFI_EXIT (ret); }static efi_status_t file_close (struct file_handle *fh)Signature · 1 line
2.18.0