[PATCH] efi: Fix memory leak in efivar_ssdt_load

[PATCH] efi: Fix memory leak in efivar_ssdt_load

[Cyrill Gorcunov]

When we load ssdt from efi variable (specified with efivar_ssdt=something
boot command line argument) a name for the variable is allocated
dynamically because we traverse all efi variables. Unlike an acpi table
data, which is later used by acpi engine, the name is no longer needed
once traverse is complete -- don't forget to free this memory.

Same time we silently ignore any errors happened here lets print
a message if something went wrong (but do not exit since this is
not a critical error and the system should continue to boot).

Also while here -- add a note why we keep ssdt table on success.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/efi.c |   38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

Index: linux-tip.git/drivers/firmware/efi/efi.c
===================================================================
--- linux-tip.git.orig/drivers/firmware/efi/efi.c
+++ linux-tip.git/drivers/firmware/efi/efi.c
@@ -273,6 +273,7 @@ static __init int efivar_ssdt_load(void)
 	efi_char16_t *name = NULL;
 	efi_status_t status;
 	efi_guid_t guid;
+	int ret = 0;
 
 	if (!efivar_ssdt[0])
 		return 0;
@@ -294,8 +295,8 @@ static __init int efivar_ssdt_load(void)
 			efi_char16_t *name_tmp =
 				krealloc(name, name_size, GFP_KERNEL);
 			if (!name_tmp) {
-				kfree(name);
-				return -ENOMEM;
+				ret = -ENOMEM;
+				goto out;
 			}
 			name = name_tmp;
 			continue;
@@ -309,26 +310,37 @@ static __init int efivar_ssdt_load(void)
 		pr_info("loading SSDT from variable %s-%pUl\n", efivar_ssdt, &guid);
 
 		status = efi.get_variable(name, &guid, NULL, &data_size, NULL);
-		if (status != EFI_BUFFER_TOO_SMALL || !data_size)
-			return -EIO;
+		if (status != EFI_BUFFER_TOO_SMALL || !data_size) {
+			ret = -EIO;
+			goto out;
+		}
 
 		data = kmalloc(data_size, GFP_KERNEL);
-		if (!data)
-			return -ENOMEM;
+		if (!data) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
 		status = efi.get_variable(name, &guid, NULL, &data_size, data);
 		if (status == EFI_SUCCESS) {
 			acpi_status ret = acpi_load_table(data, NULL);
-			if (ret)
-				pr_err("failed to load table: %u\n", ret);
-			else
+			if (ret) {
+				pr_err("efivar_ssdt: failed to load table: %u\n", ret);
+			} else {
+				/*
+				 * The @data will be in use by ACPI engine,
+				 * do not free it!
+				 */
 				continue;
+			}
 		} else {
-			pr_err("failed to get var data: 0x%lx\n", status);
+			pr_err("efivar_ssdt: failed to get var data: 0x%lx\n", status);
 		}
 		kfree(data);
 	}
-	return 0;
+out:
+	kfree(name);
+	return ret;
 }
 #else
 static inline int efivar_ssdt_load(void) { return 0; }
@@ -433,7 +445,9 @@ static int __init efisubsys_init(void)
 		error = generic_ops_register();
 		if (error)
 			goto err_put;
-		efivar_ssdt_load();
+		error = efivar_ssdt_load();
+		if (error)
+			pr_err("efi: failed to load SSDT, error %d.\n", error);
 		platform_device_register_simple("efivars", 0, NULL, 0);
 	}
 

Re: [PATCH] efi: Fix memory leak in efivar_ssdt_load

[Cyrill Gorcunov]

On Mon, Nov 04, 2024 at 02:43:46PM +0300, Cyrill Gorcunov wrote:
> When we load ssdt from efi variable (specified with efivar_ssdt=something
> boot command line argument) a name for the variable is allocated
> dynamically because we traverse all efi variables. Unlike an acpi table
> data, which is later used by acpi engine, the name is no longer needed
> once traverse is complete -- don't forget to free this memory.

Drop it for a while: found that ret variable is shadowed while compiler
didn't spit a warn about, not a bug but better fix it. Will send v2.

[PATCH v2] efi: Fix memory leak in efivar_ssdt_load

[Cyrill Gorcunov]

When we load ssdt from efi variable (specified with efivar_ssdt=something
boot command line argument) a name for the variable is allocated
dynamically because we traverse all efi variables. Unlike an acpi table
data, which is later used by acpi engine, the name is no longer needed
once traverse is complete -- don't forget to free this memory.

Same time we silently ignore any errors happened here lets print
a message if something went wrong (but do not exit since this is
not a critical error and the system should continue to boot).

Also while here -- add a note why we keep ssdt table on success.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
---
It is still unclear for me why we don't exit with error if acpi_load_table
failed but continue to iterate keys and don't report a caller with error
instead. I didn't change this logic for backward compatibility sake but
still looks suspicious.

 drivers/firmware/efi/efi.c |   41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

Index: linux-tip.git/drivers/firmware/efi/efi.c
===================================================================
--- linux-tip.git.orig/drivers/firmware/efi/efi.c
+++ linux-tip.git/drivers/firmware/efi/efi.c
@@ -273,6 +273,7 @@ static __init int efivar_ssdt_load(void)
 	efi_char16_t *name = NULL;
 	efi_status_t status;
 	efi_guid_t guid;
+	int ret = 0;
 
 	if (!efivar_ssdt[0])
 		return 0;
@@ -294,8 +295,8 @@ static __init int efivar_ssdt_load(void)
 			efi_char16_t *name_tmp =
 				krealloc(name, name_size, GFP_KERNEL);
 			if (!name_tmp) {
-				kfree(name);
-				return -ENOMEM;
+				ret = -ENOMEM;
+				goto out;
 			}
 			name = name_tmp;
 			continue;
@@ -309,26 +310,38 @@ static __init int efivar_ssdt_load(void)
 		pr_info("loading SSDT from variable %s-%pUl\n", efivar_ssdt, &guid);
 
 		status = efi.get_variable(name, &guid, NULL, &data_size, NULL);
-		if (status != EFI_BUFFER_TOO_SMALL || !data_size)
-			return -EIO;
+		if (status != EFI_BUFFER_TOO_SMALL || !data_size) {
+			ret = -EIO;
+			goto out;
+		}
 
 		data = kmalloc(data_size, GFP_KERNEL);
-		if (!data)
-			return -ENOMEM;
+		if (!data) {
+			ret = -ENOMEM;
+			goto out;
+		}
 
 		status = efi.get_variable(name, &guid, NULL, &data_size, data);
 		if (status == EFI_SUCCESS) {
-			acpi_status ret = acpi_load_table(data, NULL);
-			if (ret)
-				pr_err("failed to load table: %u\n", ret);
-			else
+			acpi_status acpi_ret = acpi_load_table(data, NULL);
+			if (ACPI_FAILURE(acpi_ret)) {
+				pr_err("efivar_ssdt: failed to load table: %u\n",
+				       acpi_ret);
+			} else {
+				/*
+				 * The @data will be in use by ACPI engine,
+				 * do not free it!
+				 */
 				continue;
+			}
 		} else {
-			pr_err("failed to get var data: 0x%lx\n", status);
+			pr_err("efivar_ssdt: failed to get var data: 0x%lx\n", status);
 		}
 		kfree(data);
 	}
-	return 0;
+out:
+	kfree(name);
+	return ret;
 }
 #else
 static inline int efivar_ssdt_load(void) { return 0; }
@@ -433,7 +446,9 @@ static int __init efisubsys_init(void)
 		error = generic_ops_register();
 		if (error)
 			goto err_put;
-		efivar_ssdt_load();
+		error = efivar_ssdt_load();
+		if (error)
+			pr_err("efi: failed to load SSDT, error %d.\n", error);
 		platform_device_register_simple("efivars", 0, NULL, 0);
 	}
 

Re: [PATCH v2] efi: Fix memory leak in efivar_ssdt_load

[Ard Biesheuvel]

On Mon, 4 Nov 2024 at 13:14, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> When we load ssdt from efi variable (specified with efivar_ssdt=something
> boot command line argument) a name for the variable is allocated
> dynamically because we traverse all efi variables. Unlike an acpi table
> data, which is later used by acpi engine, the name is no longer needed
> once traverse is complete -- don't forget to free this memory.
>
> Same time we silently ignore any errors happened here lets print
> a message if something went wrong (but do not exit since this is
> not a critical error and the system should continue to boot).
>
> Also while here -- add a note why we keep ssdt table on success.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> ---
> It is still unclear for me why we don't exit with error if acpi_load_table
> failed but continue to iterate keys and don't report a caller with error
> instead. I didn't change this logic for backward compatibility sake but
> still looks suspicious.
>

I've queued this up now.

I think there may be cases where it might make sense, or at least,
where currently, we might rely on the behavior where acpi_load_table()
failures are ignored. If a table references an object that does not
exist in the namespace (e.g., because some I2C controller is
disabled), it might still make sense to load remaining SSDTs if they
describe things that are unrelated.

But frankly, I think this is a very niche feature, and I'm not sure
supporting more than one table is that useful to begin with. You can
always merge SSDTs into a single one if needed (and things like this
are not the job of the OS imho)


>  drivers/firmware/efi/efi.c |   41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
>
> Index: linux-tip.git/drivers/firmware/efi/efi.c
> ===================================================================
> --- linux-tip.git.orig/drivers/firmware/efi/efi.c
> +++ linux-tip.git/drivers/firmware/efi/efi.c
> @@ -273,6 +273,7 @@ static __init int efivar_ssdt_load(void)
>         efi_char16_t *name = NULL;
>         efi_status_t status;
>         efi_guid_t guid;
> +       int ret = 0;
>
>         if (!efivar_ssdt[0])
>                 return 0;
> @@ -294,8 +295,8 @@ static __init int efivar_ssdt_load(void)
>                         efi_char16_t *name_tmp =
>                                 krealloc(name, name_size, GFP_KERNEL);
>                         if (!name_tmp) {
> -                               kfree(name);
> -                               return -ENOMEM;
> +                               ret = -ENOMEM;
> +                               goto out;
>                         }
>                         name = name_tmp;
>                         continue;
> @@ -309,26 +310,38 @@ static __init int efivar_ssdt_load(void)
>                 pr_info("loading SSDT from variable %s-%pUl\n", efivar_ssdt, &guid);
>
>                 status = efi.get_variable(name, &guid, NULL, &data_size, NULL);
> -               if (status != EFI_BUFFER_TOO_SMALL || !data_size)
> -                       return -EIO;
> +               if (status != EFI_BUFFER_TOO_SMALL || !data_size) {
> +                       ret = -EIO;
> +                       goto out;
> +               }
>
>                 data = kmalloc(data_size, GFP_KERNEL);
> -               if (!data)
> -                       return -ENOMEM;
> +               if (!data) {
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
>
>                 status = efi.get_variable(name, &guid, NULL, &data_size, data);
>                 if (status == EFI_SUCCESS) {
> -                       acpi_status ret = acpi_load_table(data, NULL);
> -                       if (ret)
> -                               pr_err("failed to load table: %u\n", ret);
> -                       else
> +                       acpi_status acpi_ret = acpi_load_table(data, NULL);
> +                       if (ACPI_FAILURE(acpi_ret)) {
> +                               pr_err("efivar_ssdt: failed to load table: %u\n",
> +                                      acpi_ret);
> +                       } else {
> +                               /*
> +                                * The @data will be in use by ACPI engine,
> +                                * do not free it!
> +                                */
>                                 continue;
> +                       }
>                 } else {
> -                       pr_err("failed to get var data: 0x%lx\n", status);
> +                       pr_err("efivar_ssdt: failed to get var data: 0x%lx\n", status);
>                 }
>                 kfree(data);
>         }
> -       return 0;
> +out:
> +       kfree(name);
> +       return ret;
>  }
>  #else
>  static inline int efivar_ssdt_load(void) { return 0; }
> @@ -433,7 +446,9 @@ static int __init efisubsys_init(void)
>                 error = generic_ops_register();
>                 if (error)
>                         goto err_put;
> -               efivar_ssdt_load();
> +               error = efivar_ssdt_load();
> +               if (error)
> +                       pr_err("efi: failed to load SSDT, error %d.\n", error);
>                 platform_device_register_simple("efivars", 0, NULL, 0);
>         }
>
>

Re: [PATCH v2] efi: Fix memory leak in efivar_ssdt_load

[Cyrill Gorcunov]

On Fri, Nov 15, 2024 at 06:02:07PM +0100, Ard Biesheuvel wrote:
> > It is still unclear for me why we don't exit with error if acpi_load_table
> > failed but continue to iterate keys and don't report a caller with error
> > instead. I didn't change this logic for backward compatibility sake but
> > still looks suspicious.
> >
> 
> I've queued this up now.
> 
> I think there may be cases where it might make sense, or at least,
> where currently, we might rely on the behavior where acpi_load_table()
> failures are ignored. If a table references an object that does not
> exist in the namespace (e.g., because some I2C controller is
> disabled), it might still make sense to load remaining SSDTs if they
> describe things that are unrelated.

Yeah, sound reasonable. Thanks, Ard!