[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH] efi: remove unreachable code in read_file()


  • To: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>
  • Date: Tue, 12 Aug 2025 19:17:28 +0000
  • Accept-language: en-US, uk-UA, ru-RU
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=W8qo+9Nn7D8YDP+Om+eE1Ge44AFkkk9sqUHYa6TEpKY=; b=Zw8xUlZBouWUlghrFbMXkkm+lDciM3OwWxHrK7EJX+6qRprRwaxT4k/sznVVVDux+NWwC7NZwQskP7C8Nd6EWFF6fbWYmN/+sF0irir2be1Pt7xWlHuGSqXKFux7ov2yKOh/SHRXR0VipaW7inAdcAbOE8mOYhy/JvZOf/ZOlJd41gh3g7eXUSJhstiT45aofAwrMmkvunEwbVdNtUSxgkxm9zhAzSCG8SeECOP5CCDckoqaLqlxwo79recuvS7tLx0c2El/KEZHgung9weg5SRFdrhiMe21BpUt1dNsfzxTUM6VsHnuDoIQ6o0fezBmjtkiogtPfTf65VyWeSQaMg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=nI+fWWqdkxgg6wPR71Ojz5A2067xg88Cyp4dqtlkXgRD6T1MBZ66D/DNEGpEe4fAu2cOC069tGfHHicJJLB8WUyg/yWg5eO3c+tONmqoZMQWXPaF6MAjfzwd+X2NYNLj25pfHmjyIat4wY4dKsh8GTXai05cTKMqANOcFz4tolufw/k+NFvtfoafNvJXQeh5rQRaERU3U54V9DFNmoMyNUEXqN0viVXOXWkl18VKho0Aoa+8C2EcnFgRSC3I/6PGd+EhGamgYHxOtdTdK64OR1meWm8TPM6zWO82wkZdxKMPejB+3/PLPrWIxG0NpdVN78VHOGVSXCS2/eBed/8ARg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=epam.com;
  • Cc: Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>, "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • Delivery-date: Tue, 12 Aug 2025 19:17:45 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHcC72+MqlVxdSA/E6jjiAcMI3Ndw==
  • Thread-topic: [PATCH] efi: remove unreachable code in read_file()

MISRA C Rule 2.1 states: "A project shall not contain unreachable code."

Function `PrintErrMesg(const CHAR16*, EFI_STATUS)` isn't intended to return
control to its caller. At the end, it calls `blexit()`, which, in turn,
invokes the `__builtin_unreachable()` function, making subsequent return
statements in `read_file()` unreachable:

    PrintErrMesg(name, ret);
    /* not reached */
    return false;

This also causes unreachability of the code meant to handle `read_file()`
errors, as seen in these examples:

In this block:
    if ( read_file(dir_handle, file_name, &cfg, NULL) )
        break;
    *tail = 0;
    }
    if ( !tail )
        blexit(L"No configuration file found.");

And here:
    else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
        blexit(L"Configuration file not found.");

And here:
    if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
    {
        PrintStr(L"Chained configuration file '");
        PrintStr(name.w);
        efi_bs->FreePool(name.w);
        blexit(L"'not found.");
    }

The issue arises because when an error occurs inside `read_file()`, it calls
`PrintErrMesg()` and does not return to the caller.

To address this the following changes are applied:
1. Remove `PrintErrMesg(name, ret);` from the `read_file()` function.
2. Replaced it with `PrintErr(name);`, which prints the file name and returns
   control to the caller.
3. Change the `read_file()` return type from `bool` to `EFI_STATUS`, allowing
   file operation result codes to be returned to the caller.
4. Properly handle error codes returned from the `read_file()` function in the
   relevant areas of the code.
5. Replace `blexit()` calls with informative error codes using `PrintErrMesg()`
   where appropriate.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
---
Test CI pipeline:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/pipelines/1980590118
---
 xen/common/efi/boot.c | 57 ++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 50ff1d1bd2..ddbafb2f9c 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -132,7 +132,7 @@ struct file {
     };
 };
 
-static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+static EFI_STATUS read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                       struct file *file, const char *options);
 static bool read_section(const EFI_LOADED_IMAGE *image, const CHAR16 *name,
                          struct file *file, const char *options);
@@ -782,7 +782,7 @@ static void __init handle_file_info(const CHAR16 *name,
     efi_arch_handle_module(file, name, options);
 }
 
-static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
+static EFI_STATUS __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                              struct file *file, const char *options)
 {
     EFI_FILE_HANDLE FileHandle = NULL;
@@ -791,7 +791,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
     const CHAR16 *what = NULL;
 
     if ( !name )
-        PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
+        return EFI_INVALID_PARAMETER;
 
     what = L"Open";
     if ( dir_handle )
@@ -842,7 +842,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 
     efi_arch_flush_dcache_area(file->ptr, file->size);
 
-    return true;
+    return ret;
 
  fail:
     if ( FileHandle )
@@ -850,10 +850,9 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
CHAR16 *name,
 
     PrintErr(what);
     PrintErr(L" failed for ");
-    PrintErrMesg(name, ret);
+    PrintErr(name);
 
-    /* not reached */
-    return false;
+    return ret;
 }
 
 static bool __init read_section(const EFI_LOADED_IMAGE *image,
@@ -1433,18 +1432,20 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
ImageHandle,
             while ( (tail = point_tail(file_name)) != NULL )
             {
                 wstrcpy(tail, L".cfg");
-                if ( read_file(dir_handle, file_name, &cfg, NULL) )
+                if ( (status = read_file(dir_handle, file_name,
+                                         &cfg, NULL)) == EFI_SUCCESS )
                     break;
                 *tail = 0;
             }
             if ( !tail )
-                blexit(L"No configuration file found.");
+                PrintErrMesg(L"Configuration file failure.", status);
             PrintStr(L"Using configuration file '");
             PrintStr(file_name);
             PrintStr(L"'\r\n");
         }
-        else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
-            blexit(L"Configuration file not found.");
+        else if ( (status = read_file(dir_handle, cfg_file_name,
+                                      &cfg, NULL)) != EFI_SUCCESS )
+            PrintErrMesg(L"Configuration file failure.", status);
         pre_parse(&cfg);
 
         if ( section.w )
@@ -1465,12 +1466,13 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
ImageHandle,
                 efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
                 cfg.need_to_free = false;
             }
-            if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
+            if ( (status = read_file(dir_handle, s2w(&name),
+                                     &cfg, NULL)) != EFI_SUCCESS )
             {
-                PrintStr(L"Chained configuration file '");
+                PrintStr(L"Configuration file '");
                 PrintStr(name.w);
                 efi_bs->FreePool(name.w);
-                blexit(L"'not found.");
+                PrintErrMesg(L"'failure.", status);
             }
             pre_parse(&cfg);
             efi_bs->FreePool(name.w);
@@ -1483,7 +1485,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
ImageHandle,
         if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
              name.s )
         {
-            read_file(dir_handle, s2w(&name), &kernel, option_str);
+            status = read_file(dir_handle, s2w(&name), &kernel, option_str);
+            if ( status != EFI_SUCCESS )
+            {
+                PrintStr(L"Configuration file '");
+                PrintStr(name.w);
+                efi_bs->FreePool(name.w);
+                PrintErrMesg(L"'failure.", status);
+            }
             efi_bs->FreePool(name.w);
         }
         else
@@ -1497,7 +1506,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
ImageHandle,
             name.s = get_value(&cfg, section.s, "ramdisk");
             if ( name.s )
             {
-                read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+                status = read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+                if ( status != EFI_SUCCESS )
+                {
+                    PrintStr(L"Configuration file '");
+                    PrintStr(name.w);
+                    efi_bs->FreePool(name.w);
+                    PrintErrMesg(L"'failure.", status);
+                }
                 efi_bs->FreePool(name.w);
             }
         }
@@ -1507,7 +1523,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
ImageHandle,
             name.s = get_value(&cfg, section.s, "xsm");
             if ( name.s )
             {
-                read_file(dir_handle, s2w(&name), &xsm, NULL);
+                status = read_file(dir_handle, s2w(&name), &xsm, NULL);
+                if ( status != EFI_SUCCESS )
+                {
+                    PrintStr(L"Configuration file '");
+                    PrintStr(name.w);
+                    efi_bs->FreePool(name.w);
+                    PrintErrMesg(L"'failure.", status);
+                }
                 efi_bs->FreePool(name.w);
             }
         }
-- 
2.43.0



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.