[PATCH] x86/microcode/amd: fix uninitalized structure cp

[PATCH] x86/microcode/amd: fix uninitalized structure cp

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

In the case where cp is not assigned to the return from
the call to find_microcode_in_initrd cp is uninitialized when
it is assigned to *ret.   Functions that call __load_ucode_amd
such as load_ucode_amd_bsp can therefore end up checking bogus
values cp.data and cp.size.  Fix this by ensuring cp is
initialized as all zero and remove the redundant initialization
of cp in load_ucode_amd_bsp.

Addresses-Coverity: ("Uninitialized scalar variable")
Fixes: e71bb4ec0739 ("x86/microcode/AMD: Unify load_ucode_amd_ap()")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 3f6b137ef4e6..675704019df2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -473,7 +473,7 @@ static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
 static void __load_ucode_amd(unsigned int cpuid_1_eax, struct cpio_data *ret)
 {
 	struct ucode_cpu_info *uci;
-	struct cpio_data cp;
+	struct cpio_data cp = { };
 	const char *path;
 	bool use_pa;
 
@@ -498,7 +498,7 @@ static void __load_ucode_amd(unsigned int cpuid_1_eax, struct cpio_data *ret)
 
 void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 {
-	struct cpio_data cp = { };
+	struct cpio_data cp;
 
 	__load_ucode_amd(cpuid_1_eax, &cp);
 	if (!(cp.data && cp.size))
-- 
2.24.0

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Borislav Petkov]

On Tue, Jan 14, 2020 at 11:15:05AM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> In the case where cp is not assigned to the return from
> the call to find_microcode_in_initrd

Where does this happen? I don't see it.

> cp is uninitialized when
> it is assigned to *ret.   Functions that call __load_ucode_amd
> such as load_ucode_amd_bsp can therefore end up checking bogus
> values cp.data and cp.size.  Fix this by ensuring cp is
> initialized as all zero and remove the redundant initialization
> of cp in load_ucode_amd_bsp.
> 
> Addresses-Coverity: ("Uninitialized scalar variable")

I already asked about those: either document what those tags mean or
remove them.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Colin Ian King]

On 14/01/2020 11:38, Borislav Petkov wrote:
> On Tue, Jan 14, 2020 at 11:15:05AM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> In the case where cp is not assigned to the return from
>> the call to find_microcode_in_initrd
> 
> Where does this happen? I don't see it.

Starting at load_ucode_amd_bsp(), this initializes a local cp to zero,
then passes &cp when it calls __load_ucode_amd() as parameter *ret.  In
__load_ucode_amd a new local cp is created on the stack and *only* is
assigned here:

       if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
                cp = find_microcode_in_initrd(path, use_pa);

otherwise cp is not initialized and contains garbage.  Finally, *ret is
assigned to cp:

	*ret = cp;

..and so load_ucode_amd_bsp() gets a copy of the uninitalized cp via *ret.


>> cp is uninitialized when
>> it is assigned to *ret.   Functions that call __load_ucode_amd
>> such as load_ucode_amd_bsp can therefore end up checking bogus
>> values cp.data and cp.size.  Fix this by ensuring cp is
>> initialized as all zero and remove the redundant initialization
>> of cp in load_ucode_amd_bsp.
>>
>> Addresses-Coverity: ("Uninitialized scalar variable")
> 
> I already asked about those: either document what those tags mean or
> remove them.
> 

I can send a V2 w/o these if it so pleases you. I've had nobody else
complain about these and we have literally hundreds of Coverity tagged
issues now accepted in the kernel so that we can trace how fixes are found.

Colin

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Thomas Gleixner]

Colin Ian King <colin.king@canonical.com> writes:
> On 14/01/2020 11:38, Borislav Petkov wrote:
>>>
>>> Addresses-Coverity: ("Uninitialized scalar variable")
>> 
>> I already asked about those: either document what those tags mean or
>> remove them.
>> 
> I can send a V2 w/o these if it so pleases you. I've had nobody else
> complain about these and we have literally hundreds of Coverity tagged
> issues now accepted in the kernel so that we can trace how fixes are found.

Please keep them. Having a reference how stuff got "reported" _is_
useful.

Thanks,

        tglx

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Borislav Petkov]

On Tue, Jan 14, 2020 at 11:51:43AM +0000, Colin Ian King wrote:
> Starting at load_ucode_amd_bsp(), this initializes a local cp to zero,
> then passes &cp when it calls __load_ucode_amd() as parameter *ret.  In
> __load_ucode_amd a new local cp is created on the stack and *only* is
> assigned here:
> 
>        if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
>                 cp = find_microcode_in_initrd(path, use_pa);

Is there any case where cp doesn't get assigned here? Either by
get_builtin_microcode() or by find_microcode_in_initrd()?

> I can send a V2 w/o these if it so pleases you. I've had nobody else
> complain about these and we have literally hundreds of Coverity tagged
> issues now accepted in the kernel so that we can trace how fixes are
> found.

Who's "we" and how can "we" trace them? When I see Addresses-Coverity:
how can I trace how a fix is found? How can I find out what that tag
even means?

All I'm asking is to document how one can find out what that tag means
and how it can be used by people looking at that commit message.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Colin Ian King]

On 14/01/2020 12:01, Borislav Petkov wrote:
> On Tue, Jan 14, 2020 at 11:51:43AM +0000, Colin Ian King wrote:
>> Starting at load_ucode_amd_bsp(), this initializes a local cp to zero,
>> then passes &cp when it calls __load_ucode_amd() as parameter *ret.  In
>> __load_ucode_amd a new local cp is created on the stack and *only* is
>> assigned here:
>>
>>        if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
>>                 cp = find_microcode_in_initrd(path, use_pa);
> 
> Is there any case where cp doesn't get assigned here? Either by
> get_builtin_microcode() or by find_microcode_in_initrd()?
> 
>> I can send a V2 w/o these if it so pleases you. I've had nobody else
>> complain about these and we have literally hundreds of Coverity tagged
>> issues now accepted in the kernel so that we can trace how fixes are
>> found.
> 
> Who's "we" and how can "we" trace them? When I see Addresses-Coverity:
> how can I trace how a fix is found? How can I find out what that tag
> even means?
> 
> All I'm asking is to document how one can find out what that tag means
> and how it can be used by people looking at that commit message.

OK, I will try to extract every special Tag from Coverity and get this
documented when I get some spare cycles.

> 
> Thx.
> 

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Borislav Petkov]

On Tue, Jan 14, 2020 at 12:03:36PM +0000, Colin Ian King wrote:
> On 14/01/2020 12:01, Borislav Petkov wrote:
> > On Tue, Jan 14, 2020 at 11:51:43AM +0000, Colin Ian King wrote:
> >> Starting at load_ucode_amd_bsp(), this initializes a local cp to zero,
> >> then passes &cp when it calls __load_ucode_amd() as parameter *ret.  In
> >> __load_ucode_amd a new local cp is created on the stack and *only* is
> >> assigned here:
> >>
> >>        if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
> >>                 cp = find_microcode_in_initrd(path, use_pa);
> > 
> > Is there any case where cp doesn't get assigned here? Either by
> > get_builtin_microcode() or by find_microcode_in_initrd()?
^^^^^^^^^^^^^

You missed this question.

> OK, I will try to extract every special Tag from Coverity and get this
> documented when I get some spare cycles.

tglx just explained to me the whole situation about coverity.

I'm not asking about extracting special tags but rather about a
couple of sentences somewhere in Documentation/ explaining what
Addresses-Coverity* means for the unenlightened among us and how one can
find further invormation.

Reportedly, there's even a web page with the tags somewhere...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Colin Ian King]

On 14/01/2020 12:10, Borislav Petkov wrote:
> On Tue, Jan 14, 2020 at 12:03:36PM +0000, Colin Ian King wrote:
>> On 14/01/2020 12:01, Borislav Petkov wrote:
>>> On Tue, Jan 14, 2020 at 11:51:43AM +0000, Colin Ian King wrote:
>>>> Starting at load_ucode_amd_bsp(), this initializes a local cp to zero,
>>>> then passes &cp when it calls __load_ucode_amd() as parameter *ret.  In
>>>> __load_ucode_amd a new local cp is created on the stack and *only* is
>>>> assigned here:
>>>>
>>>>        if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
>>>>                 cp = find_microcode_in_initrd(path, use_pa);
>>>
>>> Is there any case where cp doesn't get assigned here? Either by
>>> get_builtin_microcode() or by find_microcode_in_initrd()?

If I understand the question, it seems that get_builtin_microcode()
tries to load in the appropriate amd microcode binary from the cpio data
and this can potentially fail if the microcode is not provided for the
specific processor family, so I believe this is a legitimate fix.

Colin


> ^^^^^^^^^^^^^
> 
> You missed this question.
> 
>> OK, I will try to extract every special Tag from Coverity and get this
>> documented when I get some spare cycles.
> 
> tglx just explained to me the whole situation about coverity.
> 
> I'm not asking about extracting special tags but rather about a
> couple of sentences somewhere in Documentation/ explaining what
> Addresses-Coverity* means for the unenlightened among us and how one can
> find further invormation.
> 
> Reportedly, there's even a web page with the tags somewhere...
> 
> Thx.
> 

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Borislav Petkov]

On Tue, Jan 14, 2020 at 02:08:50PM +0000, Colin Ian King wrote:
> If I understand the question, it seems that get_builtin_microcode()
> tries to load in the appropriate amd microcode binary from the cpio data
> and this can potentially fail if the microcode is not provided for the
> specific processor family, so I believe this is a legitimate fix.

If the microcode for the specific processor family is not provided,
get_builtin_firmware() will return false and then we'll call
find_microcode_in_initrd() which will definitely return either a proper
pointer or a NULL-initialized cpio_data struct.

So I still don't see it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Dan Carpenter]

On Tue, Jan 14, 2020 at 04:01:53PM +0100, Borislav Petkov wrote:
> On Tue, Jan 14, 2020 at 02:08:50PM +0000, Colin Ian King wrote:
> > If I understand the question, it seems that get_builtin_microcode()
> > tries to load in the appropriate amd microcode binary from the cpio data
> > and this can potentially fail if the microcode is not provided for the
> > specific processor family, so I believe this is a legitimate fix.
> 
> If the microcode for the specific processor family is not provided,
> get_builtin_firmware() will return false and then we'll call
> find_microcode_in_initrd() which will definitely return either a proper
> pointer or a NULL-initialized cpio_data struct.
> 
> So I still don't see it.

It's probably complaining that cp.name[] isn't initialized.  UBSan will
probably generate a warning at runtime when we do:

	*ret = cp;

But otherwise it's harmless.

regards,
dan carpenter

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Borislav Petkov]

On Wed, Jan 15, 2020 at 07:25:07AM +0300, Dan Carpenter wrote:
> It's probably complaining that cp.name[] isn't initialized.

That is possible.

> UBSan will probably generate a warning at runtime when we do:
> 
> 	*ret = cp;
> 
> But otherwise it's harmless.

Yes, because we don't do anything with cpio_data.name.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode/amd: fix uninitalized structure cp

[Colin Ian King]

On 15/01/2020 12:42, Borislav Petkov wrote:
> On Wed, Jan 15, 2020 at 07:25:07AM +0300, Dan Carpenter wrote:
>> It's probably complaining that cp.name[] isn't initialized.
> 
> That is possible.
> 
>> UBSan will probably generate a warning at runtime when we do:
>>
>> 	*ret = cp;
>>
>> But otherwise it's harmless.
> 
> Yes, because we don't do anything with cpio_data.name.
> 
Yep, apologies for the noise generated by this patch, please ignore it.

Colin