2016-03-11 08:08:24

by Dan Carpenter

Subject: [patch] kexec: potetially using uninitialized variable

At the end of the function we check if "ret" has a negative error code,
but it seems possible that it is uninitialized.

Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
Signed-off-by: Dan Carpenter <[email protected]>

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 503bc2d..63d1af3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -795,7 +795,7 @@ out:

static int kexec_apply_relocations(struct kimage *image)
{
- int i, ret;
+ int i, ret = 0;
struct purgatory_info *pi = &image->purgatory_info;
Elf_Shdr *sechdrs = pi->sechdrs;

2016-03-11 08:54:33

by Xunlei Pang

Subject: Re: [patch] kexec: potetially using uninitialized variable

Hi Dan,

On 2016/03/11 at 16:07, Dan Carpenter wrote:
> At the end of the function we check if "ret" has a negative error code,
> but it seems possible that it is uninitialized.
>
> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> Signed-off-by: Dan Carpenter <[email protected]>
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 503bc2d..63d1af3 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -795,7 +795,7 @@ out:
>
> static int kexec_apply_relocations(struct kimage *image)
> {
> - int i, ret;
> + int i, ret = 0;
> struct purgatory_info *pi = &image->purgatory_info;
> Elf_Shdr *sechdrs = pi->sechdrs;
>

Look further, there is a condition at the beginning of the for loop:


if (sechdrs[i].sh_type != SHT_RELA &&
sechdrs[i].sh_type != SHT_REL)
continue;

So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.

Regards,
Xunlei

2016-03-11 09:19:46

by Dan Carpenter

Subject: Re: [patch] kexec: potetially using uninitialized variable

On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
> Hi Dan,
>
> On 2016/03/11 at 16:07, Dan Carpenter wrote:
> > At the end of the function we check if "ret" has a negative error code,
> > but it seems possible that it is uninitialized.
> >
> > Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> > Signed-off-by: Dan Carpenter <[email protected]>
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 503bc2d..63d1af3 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -795,7 +795,7 @@ out:
> >
> > static int kexec_apply_relocations(struct kimage *image)
> > {
> > - int i, ret;
> > + int i, ret = 0;
> > struct purgatory_info *pi = &image->purgatory_info;
> > Elf_Shdr *sechdrs = pi->sechdrs;
> >
>
> Look further, there is a condition at the beginning of the for loop:
>
>
> if (sechdrs[i].sh_type != SHT_RELA &&
> sechdrs[i].sh_type != SHT_REL)
> continue;
>
> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.

Ah, right...

This wasn't a GCC warning. GCC misses a lot of uninitialized variable
bugs so I'm doing this with Smatch.

Anyway, I'll patch this up in Smatch to not warn about this.

regards,
dan carpenter

2016-03-11 09:48:21

by walter harms

Subject: Re: [patch] kexec: potetially using uninitialized variable

Am 11.03.2016 10:19, schrieb Dan Carpenter:
> On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
>> Hi Dan,
>>
>> On 2016/03/11 at 16:07, Dan Carpenter wrote:
>>> At the end of the function we check if "ret" has a negative error code,
>>> but it seems possible that it is uninitialized.
>>>
>>> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
>>> Signed-off-by: Dan Carpenter <[email protected]>
>>>
>>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>>> index 503bc2d..63d1af3 100644
>>> --- a/kernel/kexec_file.c
>>> +++ b/kernel/kexec_file.c
>>> @@ -795,7 +795,7 @@ out:
>>>
>>> static int kexec_apply_relocations(struct kimage *image)
>>> {
>>> - int i, ret;
>>> + int i, ret = 0;
>>> struct purgatory_info *pi = &image->purgatory_info;
>>> Elf_Shdr *sechdrs = pi->sechdrs;
>>>
>>
>> Look further, there is a condition at the beginning of the for loop:
>>
>>
>> if (sechdrs[i].sh_type != SHT_RELA &&
>> sechdrs[i].sh_type != SHT_REL)
>> continue;
>>
>> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
>
> Ah, right...
>
> This wasn't a GCC warning. GCC misses a lot of uninitialized variable
> bugs so I'm doing this with Smatch.
>
> Anyway, I'll patch this up in Smatch to not warn about this.
>

I am not so sure about this. the point should be that the reviewer can read it easily
not if gcc complains or not.

just my 2 cents,

re,
wh

2016-03-11 15:38:19

by Minfei Huang

Subject: Re: [patch] kexec: potetially using uninitialized variable

On 03/11/16 at 10:47am, walter harms wrote:
>
>
> Am 11.03.2016 10:19, schrieb Dan Carpenter:
> > On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
> >> Hi Dan,
> >>
> >> On 2016/03/11 at 16:07, Dan Carpenter wrote:
> >>> At the end of the function we check if "ret" has a negative error code,
> >>> but it seems possible that it is uninitialized.
> >>>
> >>> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> >>> Signed-off-by: Dan Carpenter <[email protected]>
> >>>
> >>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> >>> index 503bc2d..63d1af3 100644
> >>> --- a/kernel/kexec_file.c
> >>> +++ b/kernel/kexec_file.c
> >>> @@ -795,7 +795,7 @@ out:
> >>>
> >>> static int kexec_apply_relocations(struct kimage *image)
> >>> {
> >>> - int i, ret;
> >>> + int i, ret = 0;
> >>> struct purgatory_info *pi = &image->purgatory_info;
> >>> Elf_Shdr *sechdrs = pi->sechdrs;
> >>>
> >>
> >> Look further, there is a condition at the beginning of the for loop:
> >>
> >>
> >> if (sechdrs[i].sh_type != SHT_RELA &&
> >> sechdrs[i].sh_type != SHT_REL)
> >> continue;
> >>
> >> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
> >
> > Ah, right...
> >
> > This wasn't a GCC warning. GCC misses a lot of uninitialized variable
> > bugs so I'm doing this with Smatch.
> >
> > Anyway, I'll patch this up in Smatch to not warn about this.
> >
>
> I am not so sure about this. the point should be that the reviewer can read it easily
> not if gcc complains or not.

Hi, All.

I think we can modify the logic a bit to make code simple. Thus gcc will
not complain about any more, and the logic is earier.

Following is a draft patch.

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 007b791..7144e3b 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -887,7 +887,7 @@ static int kexec_apply_relocations(struct kimage *image)
if (sechdrs[i].sh_type == SHT_RELA)
ret = arch_kexec_apply_relocations_add(pi->ehdr,
sechdrs, i);
- else if (sechdrs[i].sh_type == SHT_REL)
+ else
ret = arch_kexec_apply_relocations(pi->ehdr,
sechdrs, i);
if (ret)


>
> just my 2 cents,
>
> re,
> wh
>
>

2016-03-14 10:58:53

by Dan Carpenter

Subject: Re: [patch] kexec: potetially using uninitialized variable

On Fri, Mar 11, 2016 at 11:38:19PM +0800, Minfei Huang wrote:
> I think we can modify the logic a bit to make code simple. Thus gcc will
> not complain about any more, and the logic is earier.

This is a Smatch warning, not a GCC warning. If you think the new code
is clearer, that's fine but don't just silence the warning to please
Smatch. I'm pretty sure I can silence this warning in Smatch.

regards,
dan carpenter

2016-03-14 11:25:39

by Minfei Huang

Subject: Re: [patch] kexec: potetially using uninitialized variable

On 03/14/16 at 01:58pm, Dan Carpenter wrote:
> On Fri, Mar 11, 2016 at 11:38:19PM +0800, Minfei Huang wrote:
> > I think we can modify the logic a bit to make code simple. Thus gcc will
> > not complain about any more, and the logic is earier.
>
> This is a Smatch warning, not a GCC warning. If you think the new code
> is clearer, that's fine but don't just silence the warning to please
> Smatch. I'm pretty sure I can silence this warning in Smatch.
>
> regards,
> dan carpenter
>

Hi, Dan.

If not a GCC warning, I'm fine to fix it in Smatch, since the code logic
is clear enough.

Thanks
Minfei