efi: small leak on error
Subsystems (2)
PATCH 1/1 efi: small leak on error
The "> 0" here should ">= 0" so we free map_entries[0].
drivers/firmware/efi/runtime-map.c +1 -1
@@ -191,7 +191,7 @@ int __init efi_runtime_map_init(struct kobject *efi_kobj)return 0 ; out_add_entry: -for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }
Hi, Dan
Quoted 10 lines · Dan Carpenter
The "> 0" here should ">= 0" so we free map_entries[0].
Fixes 926172d46038 ('efi: Export EFI runtime memory mapping to sysfs')Signed-off-by Dan Carpenter <[email protected]>drivers/firmware/efi/runtime-map.c +0
-0
drivers/firmware/efi/runtime-map.c int __init efi_runtime_map_init(struct kobject *efi_kobj) · line 197
Quoted 8 lines
drivers/firmware/efi/runtime-map.c +1
-1@@ -191,7 +191,7 @@ int __init efi_runtime_map_init(struct kobject *efi_kobj)return 0 ; out_add_entry: -for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }
see below code, as for an invalid entry with i = 0, it will be not assigned to *(map_entries + i)
---
for (i = 0; i < nr_efi_runtime_map; i++) {
entry = add_sysfs_runtime_map_entry(efi_kobj, i);
if (IS_ERR(entry)) {
ret = PTR_ERR(entry);
goto out_add_entry;
}
*(map_entries + i) = entry;
}
return 0;
out_add_entry:
for (j = i - 1; j > 0; j--) {
entry = *(map_entries + j);[snip]
---
Thanks
Dave
A message that is not in the archive (sent off-list or not yet received): 20150115095455.GA15197-4/PLUo9XfK/[email protected]
Quoted 9 lines · Dave Young
drivers/firmware/efi/runtime-map.c int __init efi_runtime_map_init(struct kobject *efi_kobj) · line 197
Quoted 6 lines
out_add_entry:drivers/firmware/efi/runtime-map.c +1
-1-for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }see below code, as for an invalid entry with i = 0, it will be not assigned to *(map_entries + i)
Yes. Of course, if the first iteration fails then we want the free loop to be a noop and it is in my code as well. j = -1. -1 is not >= 0. The problem is in later iterations.
Quoted 4 lines
---
for (i = 0; i < nr_efi_runtime_map; i++) { entry = add_sysfs_runtime_map_entry(efi_kobj, i);
Assume that this is the second iteration and "i = 1".
Quoted 3 lines
if (IS_ERR(entry)) {
ret = PTR_ERR(entry);
goto out_add_entry;Assume it fails so we hit this goto. We want to free the first entry.
Quoted 8 lines
}
*(map_entries + i) = entry;
}
return 0;
out_add_entry:
for (j = i - 1; j > 0; j--) {
entry = *(map_entries + j);In your code, "j = 1 - 1" and that's not greater than zero so we don't enter this loop. In my code, we go through the loop one time.
By the way this code would be a lot more clear if you used arrays. "map_entries[j]" is more clear than "*(map_entries + j)". Even in the other patch, passing "&foo[i]" is more clear than "foo + i".
regards,
dan carpenter
Quoted 37 lines · Dan Carpenter
Quoted 9 lines · Dave Young
drivers/firmware/efi/runtime-map.c int __init efi_runtime_map_init(struct kobject *efi_kobj) · line 197
Quoted 6 lines
out_add_entry:drivers/firmware/efi/runtime-map.c +1
-1-for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }see below code, as for an invalid entry with i = 0, it will be not assigned to *(map_entries + i)
Yes. Of course, if the first iteration fails then we want the free loop to be a noop and it is in my code as well. j = -1. -1 is not >= 0. The problem is in later iterations.
Quoted 4 lines
---
for (i = 0; i < nr_efi_runtime_map; i++) { entry = add_sysfs_runtime_map_entry(efi_kobj, i);Assume that this is the second iteration and "i = 1".
Quoted 3 lines
if (IS_ERR(entry)) { ret = PTR_ERR(entry); goto out_add_entry;Assume it fails so we hit this goto. We want to free the first entry.
Quoted 8 lines
} *(map_entries + i) = entry; } return 0; out_add_entry: for (j = i - 1; j > 0; j--) { entry = *(map_entries + j);
Quoted 6 lines
In your code, "j = 1 - 1" and that's not greater than zero so we don't enter this loop. In my code, we go through the loop one time.
By the way this code would be a lot more clear if you used arrays. "map_entries[j]" is more clear than "*(map_entries + j)". Even in the other patch, passing "&foo[i]" is more clear than "foo + i".
Oops, I got your point, thanks. Will ack the patches.
I used to use the pointer, but if you want arrays, feel free to send a patch.
Thanks
Dave
Quoted 10 lines · Dan Carpenter
The "> 0" here should ">= 0" so we free map_entries[0].
Fixes 926172d46038 ('efi: Export EFI runtime memory mapping to sysfs')Signed-off-by Dan Carpenter <[email protected]>drivers/firmware/efi/runtime-map.c +0
-0
drivers/firmware/efi/runtime-map.c int __init efi_runtime_map_init(struct kobject *efi_kobj) · line 197
Quoted 8 lines
drivers/firmware/efi/runtime-map.c +1
-1@@ -191,7 +191,7 @@ int __init efi_runtime_map_init(struct kobject *efi_kobj)return 0 ; out_add_entry: -for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }
Thanks
Dave
Quoted 10 lines · Dan Carpenter
The "> 0" here should ">= 0" so we free map_entries[0].
Fixes 926172d46038 ('efi: Export EFI runtime memory mapping to sysfs')Signed-off-by Dan Carpenter <[email protected]>drivers/firmware/efi/runtime-map.c +0
-0
drivers/firmware/efi/runtime-map.c int __init efi_runtime_map_init(struct kobject *efi_kobj) · line 197
Quoted 8 lines
drivers/firmware/efi/runtime-map.c +1
-1@@ -191,7 +191,7 @@ int __init efi_runtime_map_init(struct kobject *efi_kobj)return 0 ; out_add_entry: -for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }
Thanks Dan, I've applied this for v3.20 and added Dave's ACK.
One thing: I updated the title to be a little more descriptive. Because the "efi" code has grown rather large over the past few years, it's really helpful to pinpoint exactly what area a patch is touching (in this case, it's the runtime map code).
---
From 86d68a58d00db3770735b5919ef2c6b12d7f06f3 Mon Sep 17 00:00:00 2001
From: Dan Carpenter <[email protected]>
Date: Thu, 15 Jan 2015 12:21:21 +0300
Subject: [PATCH] efi: Small leak on error in runtime map code
The "> 0" here should ">= 0" so we free map_entries[0].
--- drivers/firmware/efi/runtime-map.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/runtime-map.c +1 -1
@@ -191,7 +191,7 @@ int __init efi_runtime_map_init(struct kobject *efi_kobj)return 0 ; out_add_entry: -for (j = i -1 ; j >0 ; j--) { +for (j = i -1 ; j >=0 ; j--) { entry = *(map_entries + j);kobject_put (&entry->kobj); }
Signature · 4 lines
1.9.3 -- Matt Fleming, Intel Open Source Technology Center