[patch] efi: small leak on error

kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [patch] efi: small leak on error
@ 2015-01-15  9:21 Dan Carpenter
  2015-01-15  9:54 ` Dave Young
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-01-15  9:21 UTC (permalink / raw)
  To: Matt Fleming, Dave Young; +Cc: linux-efi, kernel-janitors

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 <dan.carpenter@oracle.com>

diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
index 018c29a..87b8e3b 100644
--- a/drivers/firmware/efi/runtime-map.c
+++ b/drivers/firmware/efi/runtime-map.c
@@ -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);
 	}

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [patch] efi: small leak on error
  2015-01-15  9:21 [patch] efi: small leak on error Dan Carpenter
@ 2015-01-15  9:54 ` Dave Young
       [not found]   ` <20150115095455.GA15197-4/PLUo9XfK/1wF9wiOj0lkEOCMrvLtNR@public.gmane.org>
  2015-01-15 12:24 ` Dave Young
  2015-01-20 15:59 ` Matt Fleming
  2 siblings, 1 reply; 6+ messages in thread
From: Dave Young @ 2015-01-15  9:54 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA

Hi, Dan

On 01/15/15 at 12:21pm, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> index 018c29a..87b8e3b 100644
> --- a/drivers/firmware/efi/runtime-map.c
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -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

^ permalink raw reply	[flat|nested] 6+ messages in thread

[parent not found: <20150115095455.GA15197-4/PLUo9XfK/1wF9wiOj0lkEOCMrvLtNR@public.gmane.org>]

* Re: [patch] efi: small leak on error
       [not found]   ` <20150115095455.GA15197-4/PLUo9XfK/1wF9wiOj0lkEOCMrvLtNR@public.gmane.org>
@ 2015-01-15 10:28     ` Dan Carpenter
  2015-01-15 12:22       ` Dave Young
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2015-01-15 10:28 UTC (permalink / raw)
  To: Dave Young
  Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA

On Thu, Jan 15, 2015 at 05:54:55PM +0800, Dave Young wrote:
> >  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) 

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.

> 
> ---
> 	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".

> 		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.

> 		}
> 		*(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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] efi: small leak on error
  2015-01-15 10:28     ` Dan Carpenter
@ 2015-01-15 12:22       ` Dave Young
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Young @ 2015-01-15 12:22 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Matt Fleming, linux-efi, kernel-janitors

On 01/15/15 at 01:28pm, Dan Carpenter wrote:
> On Thu, Jan 15, 2015 at 05:54:55PM +0800, Dave Young wrote:
> > >  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) 
> 
> 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.
> 
> > 
> > ---
> > 	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".
> 
> > 		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.
> 
> > 		}
> > 		*(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".

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] efi: small leak on error
  2015-01-15  9:21 [patch] efi: small leak on error Dan Carpenter
  2015-01-15  9:54 ` Dave Young
@ 2015-01-15 12:24 ` Dave Young
  2015-01-20 15:59 ` Matt Fleming
  2 siblings, 0 replies; 6+ messages in thread
From: Dave Young @ 2015-01-15 12:24 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Matt Fleming, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA

On 01/15/15 at 12:21pm, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> index 018c29a..87b8e3b 100644
> --- a/drivers/firmware/efi/runtime-map.c
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -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);
>  	}

Acked-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [patch] efi: small leak on error
  2015-01-15  9:21 [patch] efi: small leak on error Dan Carpenter
  2015-01-15  9:54 ` Dave Young
  2015-01-15 12:24 ` Dave Young
@ 2015-01-20 15:59 ` Matt Fleming
  2 siblings, 0 replies; 6+ messages in thread
From: Matt Fleming @ 2015-01-20 15:59 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Matt Fleming, Dave Young, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA

On Thu, 15 Jan, at 12:21:21PM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> index 018c29a..87b8e3b 100644
> --- a/drivers/firmware/efi/runtime-map.c
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -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 <dan.carpenter@oracle.com>
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].

Fixes: 926172d46038 ('efi: Export EFI runtime memory mapping to sysfs')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Dave Young <dyoung@redhat.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/firmware/efi/runtime-map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
index 018c29a26615..87b8e3b900d2 100644
--- a/drivers/firmware/efi/runtime-map.c
+++ b/drivers/firmware/efi/runtime-map.c
@@ -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);
 	}
-- 
1.9.3

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-01-20 15:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-15  9:21 [patch] efi: small leak on error Dan Carpenter
2015-01-15  9:54 ` Dave Young
     [not found]   ` <20150115095455.GA15197-4/PLUo9XfK/1wF9wiOj0lkEOCMrvLtNR@public.gmane.org>
2015-01-15 10:28     ` Dan Carpenter
2015-01-15 12:22       ` Dave Young
2015-01-15 12:24 ` Dave Young
2015-01-20 15:59 ` Matt Fleming

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).