Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Fri, Apr 23, 2021 at 9:42 AM David Laight wrote: > > From: Michael Ellerman > > Sent: 23 April 2021 14:51 > ... > > > (Does anyone - and by anyone I mean any large distro - compile with > > > local variables inited by the compiler?) > > > > This is where I say, "yes, Android" and you say "ugh no I meant a real > > distro", and I say "well ...". > > > > But yeah doesn't help us much. > > And it doesn't seem to stop my phone crashing either :-) > > Of course, I've absolutely no way of finding out where it is crashing. > Nor where the massive memory leaks are that means it need rebooting > every few days. You need a new phone. :) My Pixel3 uptime is sitting at 194 hours currently. It would be more, but those annoying security updates require reboots. I have had phones that I setup to reboot every night though. Rob
RE: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Michael Ellerman > Sent: 23 April 2021 14:51 ... > > (Does anyone - and by anyone I mean any large distro - compile with > > local variables inited by the compiler?) > > This is where I say, "yes, Android" and you say "ugh no I meant a real > distro", and I say "well ...". > > But yeah doesn't help us much. And it doesn't seem to stop my phone crashing either :-) Of course, I've absolutely no way of finding out where it is crashing. Nor where the massive memory leaks are that means it need rebooting every few days. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Daniel Axtens writes: > Daniel Axtens writes: > >> Hi Lakshmi, >> >>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: >>> >>> Sorry - missed copying device-tree and powerpc mailing lists. >>> There are a few "goto out;" statements before the local variable "fdt" is initialized through the call to of_kexec_alloc_and_setup_fdt() in elf64_load(). This will result in an uninitialized "fdt" being passed to kvfree() in this function if there is an error before the call to of_kexec_alloc_and_setup_fdt(). Initialize the local variable "fdt" to NULL. >> I'm a huge fan of initialising local variables! But I'm struggling to >> find the code path that will lead to an uninit fdt being returned... > > OK, so perhaps this was putting it too strongly. I have been bitten > by uninitialised things enough in C that I may have taken a slightly > overly-agressive view of fixing them in the source rather than the > compiler. I do think compiler-level mitigations are better, and I take > the point that we don't want to defeat compiler checking. > > (Does anyone - and by anyone I mean any large distro - compile with > local variables inited by the compiler?) This is where I say, "yes, Android" and you say "ugh no I meant a real distro", and I say "well ...". But yeah doesn't help us much. cheers
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Thu, Apr 22, 2021 at 08:05:27AM +, David Laight wrote: > > (Does anyone - and by anyone I mean any large distro - compile with > > local variables inited by the compiler?) > > There are compilers that initialise locals to zero for 'debug' builds > and leave the 'random' for optimised 'release' builds. > Lets not test what we are releasing! Yeah, that's the worst of all possible worlds. > I also think there is a new option to gcc (or clang?) to initialise > on-stack structures and arrays to ensure garbage isn't passed. > That seems to be a horrid performance hit! > Especially in userspace where large stack allocations are almost free. > > Any auto-initialise ought to be with a semi-random value > (especially not zero) so that it is never right and doesn't > lead to lazy coding. Many compilers did something like this, decades ago -- for debug builds. Segher
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Thu, Apr 22, 2021 at 08:05:27AM +, David Laight wrote: > From: Daniel Axtens > > Sent: 22 April 2021 03:21 > > > > > Hi Lakshmi, > > > > > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: > > >> > > >> Sorry - missed copying device-tree and powerpc mailing lists. > > >> > > >>> There are a few "goto out;" statements before the local variable "fdt" > > >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in > > >>> elf64_load(). This will result in an uninitialized "fdt" being passed > > >>> to kvfree() in this function if there is an error before the call to > > >>> of_kexec_alloc_and_setup_fdt(). > > >>> > > >>> Initialize the local variable "fdt" to NULL. > > >>> > > > I'm a huge fan of initialising local variables! But I'm struggling to > > > find the code path that will lead to an uninit fdt being returned... > > > > OK, so perhaps this was putting it too strongly. I have been bitten > > by uninitialised things enough in C that I may have taken a slightly > > overly-agressive view of fixing them in the source rather than the > > compiler. I do think compiler-level mitigations are better, and I take > > the point that we don't want to defeat compiler checking. > > > > (Does anyone - and by anyone I mean any large distro - compile with > > local variables inited by the compiler?) > > There are compilers that initialise locals to zero for 'debug' builds > and leave the 'random' for optimised 'release' builds. > Lets not test what we are releasing! We're eventually going to move to a world where initializing to zero it the default for the kernel. I think people will still want to initialize to a poison value for debug builds. Initializing to zero is better for debugging because it's more predictable. An it avoid information leaks. And dereferencing random uninitialized pointers is a privilege escalation but dereferencing a NULL is just an Oops. The speed impact is not very significant because (conceptually) it only needs to be done where there is a compiler warning about uninitialized variables. It's slightly more complicated in real life. In this case, the compiler doesn't know what happens inside the kexec_build_elf_info() function so it silences the warning. And GCC silences warnings if the variable is initialized inside a loop even when it doesn't know that we enter the loop. regards, dan carpenter
RE: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Daniel Axtens > Sent: 22 April 2021 03:21 > > > Hi Lakshmi, > > > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: > >> > >> Sorry - missed copying device-tree and powerpc mailing lists. > >> > >>> There are a few "goto out;" statements before the local variable "fdt" > >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in > >>> elf64_load(). This will result in an uninitialized "fdt" being passed > >>> to kvfree() in this function if there is an error before the call to > >>> of_kexec_alloc_and_setup_fdt(). > >>> > >>> Initialize the local variable "fdt" to NULL. > >>> > > I'm a huge fan of initialising local variables! But I'm struggling to > > find the code path that will lead to an uninit fdt being returned... > > OK, so perhaps this was putting it too strongly. I have been bitten > by uninitialised things enough in C that I may have taken a slightly > overly-agressive view of fixing them in the source rather than the > compiler. I do think compiler-level mitigations are better, and I take > the point that we don't want to defeat compiler checking. > > (Does anyone - and by anyone I mean any large distro - compile with > local variables inited by the compiler?) There are compilers that initialise locals to zero for 'debug' builds and leave the 'random' for optimised 'release' builds. Lets not test what we are releasing! I also think there is a new option to gcc (or clang?) to initialise on-stack structures and arrays to ensure garbage isn't passed. That seems to be a horrid performance hit! Especially in userspace where large stack allocations are almost free. Any auto-initialise ought to be with a semi-random value (especially not zero) so that it is never right and doesn't lead to lazy coding. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Daniel Axtens writes: > Hi Lakshmi, > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: >> >> Sorry - missed copying device-tree and powerpc mailing lists. >> >>> There are a few "goto out;" statements before the local variable "fdt" >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in >>> elf64_load(). This will result in an uninitialized "fdt" being passed >>> to kvfree() in this function if there is an error before the call to >>> of_kexec_alloc_and_setup_fdt(). >>> >>> Initialize the local variable "fdt" to NULL. >>> > I'm a huge fan of initialising local variables! But I'm struggling to > find the code path that will lead to an uninit fdt being returned... OK, so perhaps this was putting it too strongly. I have been bitten by uninitialised things enough in C that I may have taken a slightly overly-agressive view of fixing them in the source rather than the compiler. I do think compiler-level mitigations are better, and I take the point that we don't want to defeat compiler checking. (Does anyone - and by anyone I mean any large distro - compile with local variables inited by the compiler?) I was reading the version in powerpc/next, clearly I should have looked at linux-next. Having said that, I think I will leave the rest of the bikeshedding to the rest of you, you all seem to have it in hand :) Kind regards, Daniel > > The out label reads in part: > > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ > return ret ? ERR_PTR(ret) : fdt; > > As far as I can tell, any time we get a non-zero ret, we're going to > return an error pointer rather than the uninitialised value... > > (btw, it does look like we might leak fdt if we have an error after we > successfully kmalloc it.) > > Am I missing something? Can you link to the report for the kernel test > robot or from Dan? > > FWIW, I think it's worth including this patch _anyway_ because initing > local variables is good practice, but I'm just not sure on the > justification. > > Kind regards, > Daniel > >>> Signed-off-by: Lakshmi Ramasubramanian >>> Reported-by: kernel test robot >>> Reported-by: Dan Carpenter >>> --- >>> arch/powerpc/kexec/elf_64.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c >>> index 5a569bb51349..0051440c1f77 100644 >>> --- a/arch/powerpc/kexec/elf_64.c >>> +++ b/arch/powerpc/kexec/elf_64.c >>> @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char >>> *kernel_buf, >>> int ret; >>> unsigned long kernel_load_addr; >>> unsigned long initrd_load_addr = 0, fdt_load_addr; >>> - void *fdt; >>> + void *fdt = NULL; >>> const void *slave_code; >>> struct elfhdr ehdr; >>> char *modified_cmdline = NULL; >>> >> >> thanks, >> -lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/20/21 8:47 AM, Rob Herring wrote:
On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian
wrote:
On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
On 4/20/21 6:06 AM, Rob Herring wrote:
On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
wrote:
On 4/19/21 10:00 PM, Dan Carpenter wrote:
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
Lakshmi Ramasubramanian writes:
On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
There are a few "goto out;" statements before the local
variable "fdt"
is initialized through the call to
of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being
passed
to kvfree() in this function if there is an error before the
call to
of_kexec_alloc_and_setup_fdt().
Initialize the local variable "fdt" to NULL.
I'm a huge fan of initialising local variables! But I'm
struggling to
find the code path that will lead to an uninit fdt being
returned...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for
us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're
going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt,
but
instead sets it in the arch specific field (please see the link to
the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error
after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the
kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt.
In that
case,
* the memory cannot be freed here in case of any other
error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless
it has
already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the
error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
-goto out;
+goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
-goto out;
+goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
be freed here - it will be freed when the kexec cleanup function is
called.
That may be the case currently, but really if a function returns an
error it should have undone anything it did like memory allocations. I
don't think you should do that to fix this issue, but it would be a
good clean-up.
I agree - in case of an error the function should do a proper clean-up.
Just to be clear - for now, I will leave this as is. Correct?
Yes.
okay.
In my patch, I will do the following changes:
=> Free "fdt" when possible (as Michael had suggested in his patch)
=> Zero out "elf_info" struct at the start of the function.
Instead of zeroing out "elf_info", I think it would be better to return
an error immediately, instead of the "goto out;", if
kexec_build_elf_info() fails.
ret = kexec_build_elf_info(kernel_buf, kernel_len, , _info);
if (ret)
return ERR_PTR(ret);
I thought kexec_build_elf_info() can return an error and allocated
memory, so that would leak memory.
I looked at kexec_build_elf_info() more - it does free elf_info, if it
allocated it but encountered an error after the allocation. So it does a
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian
wrote:
>
> On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
> > On 4/20/21 6:06 AM, Rob Herring wrote:
> >> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
> >> wrote:
> >>>
> >>> On 4/19/21 10:00 PM, Dan Carpenter wrote:
> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> > Lakshmi Ramasubramanian writes:
> >> On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >>
> >>> Daniel Axtens writes:
> > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >
> > Sorry - missed copying device-tree and powerpc mailing lists.
> >
> >> There are a few "goto out;" statements before the local
> >> variable "fdt"
> >> is initialized through the call to
> >> of_kexec_alloc_and_setup_fdt() in
> >> elf64_load(). This will result in an uninitialized "fdt" being
> >> passed
> >> to kvfree() in this function if there is an error before the
> >> call to
> >> of_kexec_alloc_and_setup_fdt().
> >>
> >> Initialize the local variable "fdt" to NULL.
> >>
> I'm a huge fan of initialising local variables! But I'm
> struggling to
> find the code path that will lead to an uninit fdt being
> returned...
>
> The out label reads in part:
>
> /* Make kimage_file_post_load_cleanup free the fdt buffer for
> us. */
> return ret ? ERR_PTR(ret) : fdt;
>
> As far as I can tell, any time we get a non-zero ret, we're
> going to
> return an error pointer rather than the uninitialised value...
> >>
> >> As Dan pointed out, the new code is in linux-next.
> >>
> >> I have copied the new one below - the function doesn't return fdt,
> >> but
> >> instead sets it in the arch specific field (please see the link to
> >> the
> >> updated elf_64.c below).
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >>
> >>
>
> (btw, it does look like we might leak fdt if we have an error
> after we
> successfully kmalloc it.)
>
> Am I missing something? Can you link to the report for the
> kernel test
> robot or from Dan?
> >>
> >> /*
> >> * Once FDT buffer has been successfully passed to
> >> kexec_add_buffer(),
> >> * the FDT buffer address is saved in image->arch.fdt.
> >> In that
> >> case,
> >> * the memory cannot be freed here in case of any other
> >> error.
> >> */
> >>if (ret && !image->arch.fdt)
> >>kvfree(fdt);
> >>
> >>return ret ? ERR_PTR(ret) : NULL;
> >>
> >> In case of an error, the memory allocated for fdt is freed unless
> >> it has
> >> already been passed to kexec_add_buffer().
> >
> > It feels like the root of the problem is that the kvfree of fdt is in
> > the wrong place. It's only allocated later in the function, so the
> > error
> > path should reflect that. Something like the patch below.
> >
> > cheers
> >
> >
> > diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> > index 5a569bb51349..02662e72c53d 100644
> > --- a/arch/powerpc/kexec/elf_64.c
> > +++ b/arch/powerpc/kexec/elf_64.c
> > @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
> > char *kernel_buf,
> > ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> > initrd_len, cmdline);
> > if (ret)
> > -goto out;
> > +goto out_free_fdt;
> >
> > fdt_pack(fdt);
> >
> > @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
> > char *kernel_buf,
> > kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > ret = kexec_add_buffer();
> > if (ret)
> > -goto out;
> > +goto out_free_fdt;
> >
> > /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> > image->arch.fdt = fdt;
> > @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
> > char *kernel_buf,
> > if (ret)
> > pr_err("Error setting up the purgatory.\n");
> >
> > +goto out;
>
> This will leak. It would need to be something like:
>
> if (ret) {
> pr_err("Error setting up the purgatory.\n");
> goto out_free_fdt;
> }
> >>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
> >>> be freed here - it will be freed when the kexec cleanup function is
> >>> called.
> >>
> >> That may be the
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
On 4/20/21 6:06 AM, Rob Herring wrote:
On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
wrote:
On 4/19/21 10:00 PM, Dan Carpenter wrote:
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
Lakshmi Ramasubramanian writes:
On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
There are a few "goto out;" statements before the local
variable "fdt"
is initialized through the call to
of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being
passed
to kvfree() in this function if there is an error before the
call to
of_kexec_alloc_and_setup_fdt().
Initialize the local variable "fdt" to NULL.
I'm a huge fan of initialising local variables! But I'm
struggling to
find the code path that will lead to an uninit fdt being
returned...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for
us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're
going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt,
but
instead sets it in the arch specific field (please see the link to
the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error
after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the
kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt.
In that
case,
* the memory cannot be freed here in case of any other
error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless
it has
already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the
error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
char *kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
be freed here - it will be freed when the kexec cleanup function is
called.
That may be the case currently, but really if a function returns an
error it should have undone anything it did like memory allocations. I
don't think you should do that to fix this issue, but it would be a
good clean-up.
I agree - in case of an error the function should do a proper clean-up.
Just to be clear - for now, I will leave this as is. Correct?
In my patch, I will do the following changes:
=> Free "fdt" when possible (as Michael had suggested in his patch)
=> Zero out "elf_info" struct at the start of the function.
Instead of zeroing out "elf_info", I think it would be better to return
an error immediately, instead of the "goto out;", if
kexec_build_elf_info() fails.
ret = kexec_build_elf_info(kernel_buf, kernel_len, , _info);
if (ret)
return ERR_PTR(ret);
-lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/20/21 6:06 AM, Rob Herring wrote:
On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
wrote:
On 4/19/21 10:00 PM, Dan Carpenter wrote:
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
Lakshmi Ramasubramanian writes:
On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
There are a few "goto out;" statements before the local variable "fdt"
is initialized through the call to of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being passed
to kvfree() in this function if there is an error before the call to
of_kexec_alloc_and_setup_fdt().
Initialize the local variable "fdt" to NULL.
I'm a huge fan of initialising local variables! But I'm struggling to
find the code path that will lead to an uninit fdt being returned...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt, but
instead sets it in the arch specific field (please see the link to the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt. In that
case,
* the memory cannot be freed here in case of any other error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless it has
already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
-goto out;
+goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
-goto out;
+goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
be freed here - it will be freed when the kexec cleanup function is called.
That may be the case currently, but really if a function returns an
error it should have undone anything it did like memory allocations. I
don't think you should do that to fix this issue, but it would be a
good clean-up.
I agree - in case of an error the function should do a proper clean-up.
Just to be clear - for now, I will leave this as is. Correct?
In my patch, I will do the following changes:
=> Free "fdt" when possible (as Michael had suggested in his patch)
=> Zero out "elf_info" struct at the start of the function.
thanks,
-lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
wrote:
>
> On 4/19/21 10:00 PM, Dan Carpenter wrote:
> > On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> >> Lakshmi Ramasubramanian writes:
> >>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >>>
> Daniel Axtens writes:
> >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >>
> >> Sorry - missed copying device-tree and powerpc mailing lists.
> >>
> >>> There are a few "goto out;" statements before the local variable "fdt"
> >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >>> elf64_load(). This will result in an uninitialized "fdt" being passed
> >>> to kvfree() in this function if there is an error before the call to
> >>> of_kexec_alloc_and_setup_fdt().
> >>>
> >>> Initialize the local variable "fdt" to NULL.
> >>>
> > I'm a huge fan of initialising local variables! But I'm struggling to
> > find the code path that will lead to an uninit fdt being returned...
> >
> > The out label reads in part:
> >
> > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> > return ret ? ERR_PTR(ret) : fdt;
> >
> > As far as I can tell, any time we get a non-zero ret, we're going to
> > return an error pointer rather than the uninitialised value...
> >>>
> >>> As Dan pointed out, the new code is in linux-next.
> >>>
> >>> I have copied the new one below - the function doesn't return fdt, but
> >>> instead sets it in the arch specific field (please see the link to the
> >>> updated elf_64.c below).
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >>>
> >
> > (btw, it does look like we might leak fdt if we have an error after we
> > successfully kmalloc it.)
> >
> > Am I missing something? Can you link to the report for the kernel test
> > robot or from Dan?
> >>>
> >>> /*
> >>>* Once FDT buffer has been successfully passed to
> >>> kexec_add_buffer(),
> >>>* the FDT buffer address is saved in image->arch.fdt. In that
> >>> case,
> >>>* the memory cannot be freed here in case of any other error.
> >>>*/
> >>> if (ret && !image->arch.fdt)
> >>> kvfree(fdt);
> >>>
> >>> return ret ? ERR_PTR(ret) : NULL;
> >>>
> >>> In case of an error, the memory allocated for fdt is freed unless it has
> >>> already been passed to kexec_add_buffer().
> >>
> >> It feels like the root of the problem is that the kvfree of fdt is in
> >> the wrong place. It's only allocated later in the function, so the error
> >> path should reflect that. Something like the patch below.
> >>
> >> cheers
> >>
> >>
> >> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> >> index 5a569bb51349..02662e72c53d 100644
> >> --- a/arch/powerpc/kexec/elf_64.c
> >> +++ b/arch/powerpc/kexec/elf_64.c
> >> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
> >> *kernel_buf,
> >> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> >>initrd_len, cmdline);
> >> if (ret)
> >> -goto out;
> >> +goto out_free_fdt;
> >>
> >> fdt_pack(fdt);
> >>
> >> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char
> >> *kernel_buf,
> >> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >> ret = kexec_add_buffer();
> >> if (ret)
> >> -goto out;
> >> +goto out_free_fdt;
> >>
> >> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> >> image->arch.fdt = fdt;
> >> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
> >> *kernel_buf,
> >> if (ret)
> >> pr_err("Error setting up the purgatory.\n");
> >>
> >> +goto out;
> >
> > This will leak. It would need to be something like:
> >
> > if (ret) {
> > pr_err("Error setting up the purgatory.\n");
> > goto out_free_fdt;
> > }
> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
> be freed here - it will be freed when the kexec cleanup function is called.
That may be the case currently, but really if a function returns an
error it should have undone anything it did like memory allocations. I
don't think you should do that to fix this issue, but it would be a
good clean-up.
Rob
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/19/21 10:00 PM, Dan Carpenter wrote:
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
Lakshmi Ramasubramanian writes:
On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
There are a few "goto out;" statements before the local variable "fdt"
is initialized through the call to of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being passed
to kvfree() in this function if there is an error before the call to
of_kexec_alloc_and_setup_fdt().
Initialize the local variable "fdt" to NULL.
I'm a huge fan of initialising local variables! But I'm struggling to
find the code path that will lead to an uninit fdt being returned...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt, but
instead sets it in the arch specific field (please see the link to the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt. In that
case,
* the memory cannot be freed here in case of any other error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless it has
already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
be freed here - it will be freed when the kexec cleanup function is called.
goto out;
But we should also fix the uninitialized variable of "elf_info" if
kexec_build_elf_info() fails.
kexec_build_elf_info() frees elf_info and zeroes it in error paths,
except when elf_read_ehdr() fails. So, I think it is better to
initialize the local variable "elf_info" before calling
kexec_build_elf_info().
memset(_info, 0, sizeof(elf_info));
thanks,
-lakshmi
+
+out_free_fdt:
+ kvfree(fdt);
out:
kfree(modified_cmdline);
kexec_free_elf_info(_info);
- /*
-* Once FDT buffer has been successfully passed to kexec_add_buffer(),
-* the FDT buffer address is saved in image->arch.fdt. In that case,
-* the memory cannot be freed here in case of any other error.
-*/
- if (ret && !image->arch.fdt)
- kvfree(fdt);
-
return ret ? ERR_PTR(ret) : NULL;
}
regards,
dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> Lakshmi Ramasubramanian writes:
> > On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >
> >> Daniel Axtens writes:
> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>
> Sorry - missed copying device-tree and powerpc mailing lists.
>
> > There are a few "goto out;" statements before the local variable "fdt"
> > is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > elf64_load(). This will result in an uninitialized "fdt" being passed
> > to kvfree() in this function if there is an error before the call to
> > of_kexec_alloc_and_setup_fdt().
> >
> > Initialize the local variable "fdt" to NULL.
> >
> >>> I'm a huge fan of initialising local variables! But I'm struggling to
> >>> find the code path that will lead to an uninit fdt being returned...
> >>>
> >>> The out label reads in part:
> >>>
> >>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> >>> return ret ? ERR_PTR(ret) : fdt;
> >>>
> >>> As far as I can tell, any time we get a non-zero ret, we're going to
> >>> return an error pointer rather than the uninitialised value...
> >
> > As Dan pointed out, the new code is in linux-next.
> >
> > I have copied the new one below - the function doesn't return fdt, but
> > instead sets it in the arch specific field (please see the link to the
> > updated elf_64.c below).
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >
> >
> >>>
> >>> (btw, it does look like we might leak fdt if we have an error after we
> >>> successfully kmalloc it.)
> >>>
> >>> Am I missing something? Can you link to the report for the kernel test
> >>> robot or from Dan?
> >
> > /*
> > * Once FDT buffer has been successfully passed to
> > kexec_add_buffer(),
> > * the FDT buffer address is saved in image->arch.fdt. In that
> > case,
> > * the memory cannot be freed here in case of any other error.
> > */
> > if (ret && !image->arch.fdt)
> > kvfree(fdt);
> >
> > return ret ? ERR_PTR(ret) : NULL;
> >
> > In case of an error, the memory allocated for fdt is freed unless it has
> > already been passed to kexec_add_buffer().
>
> It feels like the root of the problem is that the kvfree of fdt is in
> the wrong place. It's only allocated later in the function, so the error
> path should reflect that. Something like the patch below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5a569bb51349..02662e72c53d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> initrd_len, cmdline);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> fdt_pack(fdt);
>
> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> ret = kexec_add_buffer();
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> image->arch.fdt = fdt;
> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> if (ret)
> pr_err("Error setting up the purgatory.\n");
>
> + goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
goto out;
But we should also fix the uninitialized variable of "elf_info" if
kexec_build_elf_info() fails.
> +
> +out_free_fdt:
> + kvfree(fdt);
> out:
> kfree(modified_cmdline);
> kexec_free_elf_info(_info);
>
> - /*
> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
> - * the FDT buffer address is saved in image->arch.fdt. In that case,
> - * the memory cannot be freed here in case of any other error.
> - */
> - if (ret && !image->arch.fdt)
> - kvfree(fdt);
> -
> return ret ? ERR_PTR(ret) : NULL;
> }
regards,
dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/19/21 4:30 PM, Michael Ellerman wrote:
Lakshmi Ramasubramanian writes:
On 4/16/21 2:05 AM, Michael Ellerman wrote:
Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
There are a few "goto out;" statements before the local variable "fdt"
is initialized through the call to of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being passed
to kvfree() in this function if there is an error before the call to
of_kexec_alloc_and_setup_fdt().
Initialize the local variable "fdt" to NULL.
I'm a huge fan of initialising local variables! But I'm struggling to
find the code path that will lead to an uninit fdt being returned...
The out label reads in part:
/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
return ret ? ERR_PTR(ret) : fdt;
As far as I can tell, any time we get a non-zero ret, we're going to
return an error pointer rather than the uninitialised value...
As Dan pointed out, the new code is in linux-next.
I have copied the new one below - the function doesn't return fdt, but
instead sets it in the arch specific field (please see the link to the
updated elf_64.c below).
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
(btw, it does look like we might leak fdt if we have an error after we
successfully kmalloc it.)
Am I missing something? Can you link to the report for the kernel test
robot or from Dan?
/*
* Once FDT buffer has been successfully passed to
kexec_add_buffer(),
* the FDT buffer address is saved in image->arch.fdt. In that
case,
* the memory cannot be freed here in case of any other error.
*/
if (ret && !image->arch.fdt)
kvfree(fdt);
return ret ? ERR_PTR(ret) : NULL;
In case of an error, the memory allocated for fdt is freed unless it has
already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
+
+out_free_fdt:
+ kvfree(fdt);
out:
kfree(modified_cmdline);
kexec_free_elf_info(_info);
- /*
-* Once FDT buffer has been successfully passed to kexec_add_buffer(),
-* the FDT buffer address is saved in image->arch.fdt. In that case,
-* the memory cannot be freed here in case of any other error.
-*/
- if (ret && !image->arch.fdt)
- kvfree(fdt);
-
return ret ? ERR_PTR(ret) : NULL;
}
This looks good to me. Thanks Michael.
I'll post the updated patch shortly.
-lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Lakshmi Ramasubramanian writes:
> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>
>> Daniel Axtens writes:
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
Sorry - missed copying device-tree and powerpc mailing lists.
> There are a few "goto out;" statements before the local variable "fdt"
> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> elf64_load(). This will result in an uninitialized "fdt" being passed
> to kvfree() in this function if there is an error before the call to
> of_kexec_alloc_and_setup_fdt().
>
> Initialize the local variable "fdt" to NULL.
>
>>> I'm a huge fan of initialising local variables! But I'm struggling to
>>> find the code path that will lead to an uninit fdt being returned...
>>>
>>> The out label reads in part:
>>>
>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
>>> return ret ? ERR_PTR(ret) : fdt;
>>>
>>> As far as I can tell, any time we get a non-zero ret, we're going to
>>> return an error pointer rather than the uninitialised value...
>
> As Dan pointed out, the new code is in linux-next.
>
> I have copied the new one below - the function doesn't return fdt, but
> instead sets it in the arch specific field (please see the link to the
> updated elf_64.c below).
>
> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>
>>>
>>> (btw, it does look like we might leak fdt if we have an error after we
>>> successfully kmalloc it.)
>>>
>>> Am I missing something? Can you link to the report for the kernel test
>>> robot or from Dan?
>
> /*
> * Once FDT buffer has been successfully passed to
> kexec_add_buffer(),
> * the FDT buffer address is saved in image->arch.fdt. In that
> case,
> * the memory cannot be freed here in case of any other error.
> */
> if (ret && !image->arch.fdt)
> kvfree(fdt);
>
> return ret ? ERR_PTR(ret) : NULL;
>
> In case of an error, the memory allocated for fdt is freed unless it has
> already been passed to kexec_add_buffer().
It feels like the root of the problem is that the kvfree of fdt is in
the wrong place. It's only allocated later in the function, so the error
path should reflect that. Something like the patch below.
cheers
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer();
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
*kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
+
+out_free_fdt:
+ kvfree(fdt);
out:
kfree(modified_cmdline);
kexec_free_elf_info(_info);
- /*
-* Once FDT buffer has been successfully passed to kexec_add_buffer(),
-* the FDT buffer address is saved in image->arch.fdt. In that case,
-* the memory cannot be freed here in case of any other error.
-*/
- if (ret && !image->arch.fdt)
- kvfree(fdt);
-
return ret ? ERR_PTR(ret) : NULL;
}
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/16/21 2:05 AM, Michael Ellerman wrote: Daniel Axtens writes: On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: Sorry - missed copying device-tree and powerpc mailing lists. There are a few "goto out;" statements before the local variable "fdt" is initialized through the call to of_kexec_alloc_and_setup_fdt() in elf64_load(). This will result in an uninitialized "fdt" being passed to kvfree() in this function if there is an error before the call to of_kexec_alloc_and_setup_fdt(). Initialize the local variable "fdt" to NULL. I'm a huge fan of initialising local variables! But I'm struggling to find the code path that will lead to an uninit fdt being returned... The out label reads in part: /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ return ret ? ERR_PTR(ret) : fdt; As far as I can tell, any time we get a non-zero ret, we're going to return an error pointer rather than the uninitialised value... As Dan pointed out, the new code is in linux-next. I have copied the new one below - the function doesn't return fdt, but instead sets it in the arch specific field (please see the link to the updated elf_64.c below). https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next (btw, it does look like we might leak fdt if we have an error after we successfully kmalloc it.) Am I missing something? Can you link to the report for the kernel test robot or from Dan? /* * Once FDT buffer has been successfully passed to kexec_add_buffer(), * the FDT buffer address is saved in image->arch.fdt. In that case, * the memory cannot be freed here in case of any other error. */ if (ret && !image->arch.fdt) kvfree(fdt); return ret ? ERR_PTR(ret) : NULL; In case of an error, the memory allocated for fdt is freed unless it has already been passed to kexec_add_buffer(). thanks, -lakshmi FWIW, I think it's worth including this patch _anyway_ because initing local variables is good practice, but I'm just not sure on the justification. Why is it good practice? It defeats -Wuninitialized. So you're guaranteed to be returning something initialised, but not necessarily initialised to the right value. In a case like this NULL seems like a safe choice, but it's still wrong. The function is meant to return a pointer to the successfully allocated fdt, or an ERR_PTR() value. NULL is neither of those. I agree there are security reasons that initialising stack variables is desirable, but I think that should be handled by the compiler, not at the source level. cheers
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Dan Carpenter writes:
> On Fri, Apr 16, 2021 at 09:00:12AM +0200, Christophe Leroy wrote:
>> Le 16/04/2021 à 08:44, Daniel Axtens a écrit :
>> > > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>> > >
>> > > > There are a few "goto out;" statements before the local variable "fdt"
>> > > > is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>> > > > elf64_load(). This will result in an uninitialized "fdt" being passed
>> > > > to kvfree() in this function if there is an error before the call to
>> > > > of_kexec_alloc_and_setup_fdt().
>> > > >
>> > > > Initialize the local variable "fdt" to NULL.
>> > > >
>> > I'm a huge fan of initialising local variables! But I'm struggling to
>> > find the code path that will lead to an uninit fdt being returned...
>> >
>> > The out label reads in part:
>> >
>> >/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
>> >return ret ? ERR_PTR(ret) : fdt;
>> >
>> > As far as I can tell, any time we get a non-zero ret, we're going to
>> > return an error pointer rather than the uninitialised value...
>>
>> I don't think GCC is smart enough to detect that.
>>
>
> We disabled uninitialized variable checking for GCC.
We disabled -Wmaybe-uninitialized, but that doesn't disable *all*
uninitialized warnings does it?
I wish we hadn't disabled it, it's already led to bugs slipping through.
> But actually is something that has been on my mind recently. Smatch is
> supposed to parse this correctly but there is a bug that affects powerpc
> and I don't know how to debug it. The kbuild bot is doing cross
> platform compiles but I don't have one set up on myself. Could someone
> with Smatch installed test something for me?
>
> Or if you don't have Smatch installed then you should definitely install
> it. :P
> https://www.spinics.net/lists/smatch/msg00568.html
I have smatch installed, and even run it sometimes ;)
> Apply the patch from below and edit the path to point to the correct
> directory. Then run kchecker and email me the output?
That gave me:
CC arch/powerpc/kernel/hw_breakpoint.o
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c: In function
‘task_bps_add’:
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:176:16: error:
passing argument 1 of ‘__smatch_about’ makes integer from pointer without a
cast [-Werror=int-conversion]
176 | __smatch_about(tmp);
|^~~
||
|struct breakpoint *
In file included from
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:170:
/home/michael/smatch/check_debug.h:4:40: note: expected ‘long int’ but
argument is of type ‘struct breakpoint *’
4 | static inline void __smatch_about(long var){}
| ~^~~
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:180:21: error:
passing argument 1 of ‘__smatch_about’ makes integer from pointer without a
cast [-Werror=int-conversion]
180 | __smatch_about(tmp);
| ^~~
| |
| struct breakpoint *
In file included from
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:170:
/home/michael/smatch/check_debug.h:4:40: note: expected ‘long int’ but
argument is of type ‘struct breakpoint *’
4 | static inline void __smatch_about(long var){}
| ~^~~
cc1: all warnings being treated as errors
Which looks like it didn't work.
Right, needs tmp cast to long.
Output below, hope it helps. Happy to test other things.
cheers
GEN Makefile
CHECK /home/michael/linux/scripts/mod/empty.c
CALL/home/michael/linux/scripts/checksyscalls.sh
CALL/home/michael/linux/scripts/atomic/check-atomics.sh
CHECK /home/michael/linux/arch/powerpc/kernel/vdso64/vgettimeofday.c
CC arch/powerpc/kernel/hw_breakpoint.o
CHECK /home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add()
about
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add()
implied: tmp = 's64min-(-4096),(-12),4096-s64max'
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add() buf
size: 'tmp' 0 elements, 0 bytes (rl = (-1),32)
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add()
strlen: 'tmp' characters
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add() real
absolute: tmp = 's64min-(-4096),(-12),4096-s64max'
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add() mtag
= 0 offset = 0 rl = ''
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add()
[register_smatch_extra] tmp = '4096-ptr_max,(-12)' [merged]
(4096-ptr_max,(-12), 4096-ptr_max, (-12), 4096-ptr_max, (-12))
/home/michael/linux/arch/powerpc/kernel/hw_breakpoint.c:177 task_bps_add()
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Daniel Axtens writes: >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: >> >> Sorry - missed copying device-tree and powerpc mailing lists. >> >>> There are a few "goto out;" statements before the local variable "fdt" >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in >>> elf64_load(). This will result in an uninitialized "fdt" being passed >>> to kvfree() in this function if there is an error before the call to >>> of_kexec_alloc_and_setup_fdt(). >>> >>> Initialize the local variable "fdt" to NULL. >>> > I'm a huge fan of initialising local variables! But I'm struggling to > find the code path that will lead to an uninit fdt being returned... > > The out label reads in part: > > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ > return ret ? ERR_PTR(ret) : fdt; > > As far as I can tell, any time we get a non-zero ret, we're going to > return an error pointer rather than the uninitialised value... > > (btw, it does look like we might leak fdt if we have an error after we > successfully kmalloc it.) > > Am I missing something? Can you link to the report for the kernel test > robot or from Dan? > > FWIW, I think it's worth including this patch _anyway_ because initing > local variables is good practice, but I'm just not sure on the > justification. Why is it good practice? It defeats -Wuninitialized. So you're guaranteed to be returning something initialised, but not necessarily initialised to the right value. In a case like this NULL seems like a safe choice, but it's still wrong. The function is meant to return a pointer to the successfully allocated fdt, or an ERR_PTR() value. NULL is neither of those. I agree there are security reasons that initialising stack variables is desirable, but I think that should be handled by the compiler, not at the source level. cheers
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Fri, Apr 16, 2021 at 09:00:12AM +0200, Christophe Leroy wrote:
>
>
> Le 16/04/2021 à 08:44, Daniel Axtens a écrit :
> > Hi Lakshmi,
> >
> > > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> > >
> > > Sorry - missed copying device-tree and powerpc mailing lists.
> > >
> > > > There are a few "goto out;" statements before the local variable "fdt"
> > > > is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > > > elf64_load(). This will result in an uninitialized "fdt" being passed
> > > > to kvfree() in this function if there is an error before the call to
> > > > of_kexec_alloc_and_setup_fdt().
> > > >
> > > > Initialize the local variable "fdt" to NULL.
> > > >
> > I'm a huge fan of initialising local variables! But I'm struggling to
> > find the code path that will lead to an uninit fdt being returned...
> >
> > The out label reads in part:
> >
> > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> > return ret ? ERR_PTR(ret) : fdt;
> >
> > As far as I can tell, any time we get a non-zero ret, we're going to
> > return an error pointer rather than the uninitialised value...
>
> I don't think GCC is smart enough to detect that.
>
We disabled uninitialized variable checking for GCC.
But actually is something that has been on my mind recently. Smatch is
supposed to parse this correctly but there is a bug that affects powerpc
and I don't know how to debug it. The kbuild bot is doing cross
platform compiles but I don't have one set up on myself. Could someone
with Smatch installed test something for me?
Or if you don't have Smatch installed then you should definitely install
it. :P
https://www.spinics.net/lists/smatch/msg00568.html
Apply the patch from below and edit the path to point to the correct
directory. Then run kchecker and email me the output?
~/path/to/smatch_scripts/kchecker arch/powerpc/kernel/hw_breakpoint.c
regads,
dan carpenter
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
index 8fc7a14e4d71..f2dfba54e14d 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -167,13 +167,19 @@ static bool can_co_exist(struct breakpoint *b, struct
perf_event *bp)
return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
}
+#include "/home/XXX/path/to/smatch/check_debug.h"
static int task_bps_add(struct perf_event *bp)
{
struct breakpoint *tmp;
tmp = alloc_breakpoint(bp);
- if (IS_ERR(tmp))
+ __smatch_about(tmp);
+ __smatch_debug_on();
+ if (IS_ERR(tmp)) {
+ __smatch_debug_off();
+ __smatch_about(tmp);
return PTR_ERR(tmp);
+ }
list_add(>list, _bps);
return 0;
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Fri, Apr 16, 2021 at 04:44:30PM +1000, Daniel Axtens wrote:
> Hi Lakshmi,
>
> > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >
> > Sorry - missed copying device-tree and powerpc mailing lists.
> >
> >> There are a few "goto out;" statements before the local variable "fdt"
> >> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >> elf64_load(). This will result in an uninitialized "fdt" being passed
> >> to kvfree() in this function if there is an error before the call to
> >> of_kexec_alloc_and_setup_fdt().
> >>
> >> Initialize the local variable "fdt" to NULL.
> >>
> I'm a huge fan of initialising local variables!
Don't be! It just disables static checker warnings and hides bugs.
The kbuild emails are archived but the email is mangled and unreadable.
https://www.mail-archive.com/kbuild@lists.01.org/msg06371.html
I think maybe you're not on the most recent code. In linux-next this
code looks like:
arch/powerpc/kexec/elf_64.c
27 static void *elf64_load(struct kimage *image, char *kernel_buf,
28 unsigned long kernel_len, char *initrd,
29 unsigned long initrd_len, char *cmdline,
30 unsigned long cmdline_len)
31 {
32 int ret;
33 unsigned long kernel_load_addr;
34 unsigned long initrd_load_addr = 0, fdt_load_addr;
35 void *fdt;
36 const void *slave_code;
37 struct elfhdr ehdr;
38 char *modified_cmdline = NULL;
39 struct kexec_elf_info elf_info;
40 struct kexec_buf kbuf = { .image = image, .buf_min = 0,
41.buf_max = ppc64_rma_size };
42 struct kexec_buf pbuf = { .image = image, .buf_min = 0,
43.buf_max = ppc64_rma_size, .top_down
= true,
44.mem = KEXEC_BUF_MEM_UNKNOWN };
45
46 ret = kexec_build_elf_info(kernel_buf, kernel_len, ,
_info);
47 if (ret)
48 goto out;
I really despise "goto out;" because freeing things which haven't been
allocated is always dangerous.
[ snip ].
143 out:
144 kfree(modified_cmdline);
145 kexec_free_elf_info(_info);
There is a possibility that "elf_info" has holds uninitialized stack
data if elf_read_ehdr() fails so that's probably fixing as well. kexec()
is root only so this can't be exploited.
146
147 /*
148 * Once FDT buffer has been successfully passed to
kexec_add_buffer(),
149 * the FDT buffer address is saved in image->arch.fdt. In that
case,
150 * the memory cannot be freed here in case of any other error.
151 */
152 if (ret && !image->arch.fdt)
153 kvfree(fdt);
^^^
Uninitialized.
154
155 return ret ? ERR_PTR(ret) : NULL;
156 }
regards,
dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Le 16/04/2021 à 08:44, Daniel Axtens a écrit : Hi Lakshmi, On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: Sorry - missed copying device-tree and powerpc mailing lists. There are a few "goto out;" statements before the local variable "fdt" is initialized through the call to of_kexec_alloc_and_setup_fdt() in elf64_load(). This will result in an uninitialized "fdt" being passed to kvfree() in this function if there is an error before the call to of_kexec_alloc_and_setup_fdt(). Initialize the local variable "fdt" to NULL. I'm a huge fan of initialising local variables! But I'm struggling to find the code path that will lead to an uninit fdt being returned... The out label reads in part: /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ return ret ? ERR_PTR(ret) : fdt; As far as I can tell, any time we get a non-zero ret, we're going to return an error pointer rather than the uninitialised value... I don't think GCC is smart enough to detect that. (btw, it does look like we might leak fdt if we have an error after we successfully kmalloc it.) Am I missing something? Can you link to the report for the kernel test robot or from Dan? FWIW, I think it's worth including this patch _anyway_ because initing local variables is good practice, but I'm just not sure on the justification. I don't think local systematically initing local variables is a good practice at all, as it leads to bugs where you get a wrong value because of pathes where you forgot to set the correct value. If you don't init local variable at declaration and forget to set it in some pathes, the compiler will detect it and warn you. If you init the local variable with an arbitrary value at declaration and forget to set it later in some pathes, the compiler won't be able to detect it and you will go with the wrong value. Christophe Kind regards, Daniel Signed-off-by: Lakshmi Ramasubramanian Reported-by: kernel test robot Reported-by: Dan Carpenter --- arch/powerpc/kexec/elf_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c index 5a569bb51349..0051440c1f77 100644 --- a/arch/powerpc/kexec/elf_64.c +++ b/arch/powerpc/kexec/elf_64.c @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, int ret; unsigned long kernel_load_addr; unsigned long initrd_load_addr = 0, fdt_load_addr; - void *fdt; + void *fdt = NULL; const void *slave_code; struct elfhdr ehdr; char *modified_cmdline = NULL; thanks, -lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
Hi Lakshmi, > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: > > Sorry - missed copying device-tree and powerpc mailing lists. > >> There are a few "goto out;" statements before the local variable "fdt" >> is initialized through the call to of_kexec_alloc_and_setup_fdt() in >> elf64_load(). This will result in an uninitialized "fdt" being passed >> to kvfree() in this function if there is an error before the call to >> of_kexec_alloc_and_setup_fdt(). >> >> Initialize the local variable "fdt" to NULL. >> I'm a huge fan of initialising local variables! But I'm struggling to find the code path that will lead to an uninit fdt being returned... The out label reads in part: /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ return ret ? ERR_PTR(ret) : fdt; As far as I can tell, any time we get a non-zero ret, we're going to return an error pointer rather than the uninitialised value... (btw, it does look like we might leak fdt if we have an error after we successfully kmalloc it.) Am I missing something? Can you link to the report for the kernel test robot or from Dan? FWIW, I think it's worth including this patch _anyway_ because initing local variables is good practice, but I'm just not sure on the justification. Kind regards, Daniel >> Signed-off-by: Lakshmi Ramasubramanian >> Reported-by: kernel test robot >> Reported-by: Dan Carpenter >> --- >> arch/powerpc/kexec/elf_64.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c >> index 5a569bb51349..0051440c1f77 100644 >> --- a/arch/powerpc/kexec/elf_64.c >> +++ b/arch/powerpc/kexec/elf_64.c >> @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char >> *kernel_buf, >> int ret; >> unsigned long kernel_load_addr; >> unsigned long initrd_load_addr = 0, fdt_load_addr; >> -void *fdt; >> +void *fdt = NULL; >> const void *slave_code; >> struct elfhdr ehdr; >> char *modified_cmdline = NULL; >> > > thanks, > -lakshmi
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: Sorry - missed copying device-tree and powerpc mailing lists. There are a few "goto out;" statements before the local variable "fdt" is initialized through the call to of_kexec_alloc_and_setup_fdt() in elf64_load(). This will result in an uninitialized "fdt" being passed to kvfree() in this function if there is an error before the call to of_kexec_alloc_and_setup_fdt(). Initialize the local variable "fdt" to NULL. Signed-off-by: Lakshmi Ramasubramanian Reported-by: kernel test robot Reported-by: Dan Carpenter --- arch/powerpc/kexec/elf_64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c index 5a569bb51349..0051440c1f77 100644 --- a/arch/powerpc/kexec/elf_64.c +++ b/arch/powerpc/kexec/elf_64.c @@ -32,7 +32,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, int ret; unsigned long kernel_load_addr; unsigned long initrd_load_addr = 0, fdt_load_addr; - void *fdt; + void *fdt = NULL; const void *slave_code; struct elfhdr ehdr; char *modified_cmdline = NULL; thanks, -lakshmi
