The code here prints an error if "p" is an error pointer but it still
dereferences it at the end of the function when it does:
intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
We can just return early instead.
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: return early instead checking for IS_ERR_OR_NULL() at the end.
We have to keep the final check whether "p" is NULL to handle the
situation were we set "prev_found = true;" but then hit the continue
statement instead of allocating "p".
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9d4ccb..aab900ddea6a 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -184,10 +184,11 @@ static void save_microcode_patch(void *data, unsigned int size)
continue;
p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ if (IS_ERR(p)) {
pr_err("Error allocating buffer %p\n", data);
- else
- list_replace(&iter->plist, &p->plist);
+ return;
+ }
+ list_replace(&iter->plist, &p->plist);
}
}
@@ -197,10 +198,11 @@ static void save_microcode_patch(void *data, unsigned int size)
*/
if (!prev_found) {
p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ if (IS_ERR(p)) {
pr_err("Error allocating buffer for %p\n", data);
- else
- list_add_tail(&p->plist, µcode_cache);
+ return;
+ }
+ list_add_tail(&p->plist, µcode_cache);
}
/*
On Thu, Aug 24, 2017 at 11:15:57PM +0300, Dan Carpenter wrote:
> The code here prints an error if "p" is an error pointer but it still
> dereferences it at the end of the function when it does:
>
> intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
>
> We can just return early instead.
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> v2: return early instead checking for IS_ERR_OR_NULL() at the end.
>
> We have to keep the final check whether "p" is NULL to handle the
> situation were we set "prev_found = true;" but then hit the continue
> statement instead of allocating "p".
I think we want to something more like this (not exit the loop if the
allocation fails). But I need to look at the again on a clear head:a
---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9d4ccb..0179f0fd8a79 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -146,7 +146,7 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
return false;
}
-static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
+static struct ucode_patch *memdup_patch(void *data, unsigned int size)
{
struct ucode_patch *p;
@@ -183,11 +183,13 @@ static void save_microcode_patch(void *data, unsigned int size)
if (mc_hdr->rev <= mc_saved_hdr->rev)
continue;
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (IS_ERR(p)) {
pr_err("Error allocating buffer %p\n", data);
- else
- list_replace(&iter->plist, &p->plist);
+ continue;
+ }
+
+ list_replace(&iter->plist, &p->plist);
}
}
@@ -196,11 +198,12 @@ static void save_microcode_patch(void *data, unsigned int size)
* newly found.
*/
if (!prev_found) {
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (IS_ERR(p)) {
pr_err("Error allocating buffer for %p\n", data);
- else
- list_add_tail(&p->plist, µcode_cache);
+ return;
+ }
+ list_add_tail(&p->plist, µcode_cache);
}
/*
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Thu, Aug 24, 2017 at 10:47:14PM +0200, Borislav Petkov wrote:
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 59edbe9d4ccb..0179f0fd8a79 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -146,7 +146,7 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
> return false;
> }
>
> -static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
> +static struct ucode_patch *memdup_patch(void *data, unsigned int size)
> {
> struct ucode_patch *p;
>
> @@ -183,11 +183,13 @@ static void save_microcode_patch(void *data, unsigned int size)
> if (mc_hdr->rev <= mc_saved_hdr->rev)
> continue;
>
> - p = __alloc_microcode_buf(data, size);
> - if (IS_ERR(p))
> + p = memdup_patch(data, size);
> + if (IS_ERR(p)) {
> pr_err("Error allocating buffer %p\n", data);
> - else
> - list_replace(&iter->plist, &p->plist);
> + continue;
> + }
> +
> + list_replace(&iter->plist, &p->plist);
> }
> }
>
This is just cleanups and doesn't change the behavior.
> @@ -196,11 +198,12 @@ static void save_microcode_patch(void *data, unsigned int size)
> * newly found.
> */
> if (!prev_found) {
> - p = __alloc_microcode_buf(data, size);
> - if (IS_ERR(p))
> + p = memdup_patch(data, size);
> + if (IS_ERR(p)) {
> pr_err("Error allocating buffer for %p\n", data);
> - else
> - list_add_tail(&p->plist, µcode_cache);
> + return;
> + }
> + list_add_tail(&p->plist, µcode_cache);
> }
The static checker is still going to complain about the error pointer
from the loop. Perhaps we should only set prev_found if the memdup_patch()
inside the loop succeeds?
regards,
dan carpenter
On Thu, Aug 24, 2017 at 11:55:10PM +0300, Dan Carpenter wrote:
> This is just cleanups and doesn't change the behavior.
You can't return from in the middle of the loop just because the
allocation fails.
> The static checker is still going to complain about the error pointer
> from the loop.
Please drop this argument about the static checker which you write. I'm
certainly not changing code just because some tool complains.
> Perhaps we should only set prev_found if the memdup_patch()
> inside the loop succeeds?
This not why we set prev_found.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Thu, 2017-08-24 at 22:47 +0200, Borislav Petkov wrote:
> On Thu, Aug 24, 2017 at 11:15:57PM +0300, Dan Carpenter wrote:
> > The code here prints an error if "p" is an error pointer but it still
> > dereferences it at the end of the function when it does:
> >
> > intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
> >
> > We can just return early instead.
> >
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > v2: return early instead checking for IS_ERR_OR_NULL() at the end.
> >
> > We have to keep the final check whether "p" is NULL to handle the
> > situation were we set "prev_found = true;" but then hit the continue
> > statement instead of allocating "p".
>
> I think we want to something more like this (not exit the loop if the
> allocation fails). But I need to look at the again on a clear head:a
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
[]
> @@ -146,7 +146,7 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
> return false;
> }
>
> -static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
> +static struct ucode_patch *memdup_patch(void *data, unsigned int size)
> {
> struct ucode_patch *p;
>
> @@ -183,11 +183,13 @@ static void save_microcode_patch(void *data, unsigned int size)
> if (mc_hdr->rev <= mc_saved_hdr->rev)
> continue;
>
> - p = __alloc_microcode_buf(data, size);
> - if (IS_ERR(p))
> + p = memdup_patch(data, size);
> + if (IS_ERR(p)) {
> pr_err("Error allocating buffer %p\n", data);
The pr_err could also be deleted as memdup_patch also
just does normal allocations without __GFP_NOWARN so
dump_stack() still would occur.
On Thu, Aug 24, 2017 at 10:58:44PM +0200, Borislav Petkov wrote:
> On Thu, Aug 24, 2017 at 11:55:10PM +0300, Dan Carpenter wrote:
> > This is just cleanups and doesn't change the behavior.
>
> You can't return from in the middle of the loop just because the
> allocation fails.
>
I understand that.
> > The static checker is still going to complain about the error pointer
> > from the loop.
>
> Please drop this argument about the static checker which you write. I'm
> certainly not changing code just because some tool complains.
Sure. But the point is the same... The "p" is an error pointer at the
end of the function.
>
> > Perhaps we should only set prev_found if the memdup_patch()
> > inside the loop succeeds?
>
> This not why we set prev_found.
Sure.
regards,
dan carpenter
On Fri, Aug 25, 2017 at 12:08:47AM +0300, Dan Carpenter wrote:
> Sure. But the point is the same... The "p" is an error pointer at the
> end of the function.
Of course, that's why I'm still trying to find a solution. I'm just not
happy with what the ones we have now. I'll think about it more tomorrow,
on a clear head.
Thanks.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Ok,
your initial patch was on the right track but let's simplify the whole
flow. That should not trigger your checker warning anymore:
---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9d4ccb..8f7a9bbad514 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -146,18 +146,18 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
return false;
}
-static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
+static struct ucode_patch *memdup_patch(void *data, unsigned int size)
{
struct ucode_patch *p;
p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
if (!p)
- return ERR_PTR(-ENOMEM);
+ return NULL;
p->data = kmemdup(data, size, GFP_KERNEL);
if (!p->data) {
kfree(p);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
return p;
@@ -183,8 +183,8 @@ static void save_microcode_patch(void *data, unsigned int size)
if (mc_hdr->rev <= mc_saved_hdr->rev)
continue;
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer %p\n", data);
else
list_replace(&iter->plist, &p->plist);
@@ -196,24 +196,25 @@ static void save_microcode_patch(void *data, unsigned int size)
* newly found.
*/
if (!prev_found) {
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer for %p\n", data);
else
list_add_tail(&p->plist, µcode_cache);
}
+ if (!p)
+ return;
+
/*
* Save for early loading. On 32-bit, that needs to be a physical
* address as the APs are running from physical addresses, before
* paging has been enabled.
*/
- if (p) {
- if (IS_ENABLED(CONFIG_X86_32))
- intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
- else
- intel_ucode_patch = p->data;
- }
+ if (IS_ENABLED(CONFIG_X86_32))
+ intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
+ else
+ intel_ucode_patch = p->data;
}
static int microcode_sanity_check(void *mc, int print_err)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
That patch looks good to me. Could you just add a changelog and give
me a Reported-by: credit?
regards,
dan carpenter
On Fri, Aug 25, 2017 at 12:12:28PM +0300, Dan Carpenter wrote:
> That patch looks good to me. Could you just add a changelog and give
> me a Reported-by: credit?
Of course.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
From: Borislav Petkov <[email protected]>
Avoid potentially dereferencing a NULL pointer when saving a microcode
patch for early loading on the application processors.
While at it, drop the IS_ERR() checking in favor of simpler, NULL-ptr
checks which are sufficient and rename __alloc_microcode_buf() to
memdup_patch() to more precisely denote what it does.
No functionality change.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9d4ccb..8f7a9bbad514 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -146,18 +146,18 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
return false;
}
-static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
+static struct ucode_patch *memdup_patch(void *data, unsigned int size)
{
struct ucode_patch *p;
p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
if (!p)
- return ERR_PTR(-ENOMEM);
+ return NULL;
p->data = kmemdup(data, size, GFP_KERNEL);
if (!p->data) {
kfree(p);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
return p;
@@ -183,8 +183,8 @@ static void save_microcode_patch(void *data, unsigned int size)
if (mc_hdr->rev <= mc_saved_hdr->rev)
continue;
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer %p\n", data);
else
list_replace(&iter->plist, &p->plist);
@@ -196,24 +196,25 @@ static void save_microcode_patch(void *data, unsigned int size)
* newly found.
*/
if (!prev_found) {
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer for %p\n", data);
else
list_add_tail(&p->plist, µcode_cache);
}
+ if (!p)
+ return;
+
/*
* Save for early loading. On 32-bit, that needs to be a physical
* address as the APs are running from physical addresses, before
* paging has been enabled.
*/
- if (p) {
- if (IS_ENABLED(CONFIG_X86_32))
- intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
- else
- intel_ucode_patch = p->data;
- }
+ if (IS_ENABLED(CONFIG_X86_32))
+ intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
+ else
+ intel_ucode_patch = p->data;
}
static int microcode_sanity_check(void *mc, int print_err)
--
2.13.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Am 25.08.2017 12:04, schrieb Borislav Petkov:
> From: Borislav Petkov <[email protected]>
>
> Avoid potentially dereferencing a NULL pointer when saving a microcode
> patch for early loading on the application processors.
>
> While at it, drop the IS_ERR() checking in favor of simpler, NULL-ptr
> checks which are sufficient and rename __alloc_microcode_buf() to
> memdup_patch() to more precisely denote what it does.
>
> No functionality change.
>
> Reported-by: Dan Carpenter <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 59edbe9d4ccb..8f7a9bbad514 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -146,18 +146,18 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
> return false;
> }
>
> -static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
> +static struct ucode_patch *memdup_patch(void *data, unsigned int size)
> {
> struct ucode_patch *p;
>
> p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
> if (!p)
> - return ERR_PTR(-ENOMEM);
> + return NULL;
>
> p->data = kmemdup(data, size, GFP_KERNEL);
> if (!p->data) {
> kfree(p);
> - return ERR_PTR(-ENOMEM);
> + return NULL;
> }
>
> return p;
> @@ -183,8 +183,8 @@ static void save_microcode_patch(void *data, unsigned int size)
> if (mc_hdr->rev <= mc_saved_hdr->rev)
> continue;
>
> - p = __alloc_microcode_buf(data, size);
> - if (IS_ERR(p))
> + p = memdup_patch(data, size);
> + if (!p)
> pr_err("Error allocating buffer %p\n", data);
> else
> list_replace(&iter->plist, &p->plist);
> @@ -196,24 +196,25 @@ static void save_microcode_patch(void *data, unsigned int size)
> * newly found.
> */
> if (!prev_found) {
> - p = __alloc_microcode_buf(data, size);
> - if (IS_ERR(p))
> + p = memdup_patch(data, size);
> + if (!p)
> pr_err("Error allocating buffer for %p\n", data);
> else
> list_add_tail(&p->plist, µcode_cache);
> }
>
> + if (!p)
> + return;
> +
just a bit nitpicking,
i would expect something like that:
p = memdup_patch(data, size);
if (!p) {
pr_err("Error allocating buffer for %p\n", data);
return;
}
list_add_tail(&p->plist, µcode_cache);
... because this is a normal pattern for OOF conditions and
everyone will ask "Why continue when there is no memory"
just my 2 cents
re,
wh
> /*
> * Save for early loading. On 32-bit, that needs to be a physical
> * address as the APs are running from physical addresses, before
> * paging has been enabled.
> */
> - if (p) {
> - if (IS_ENABLED(CONFIG_X86_32))
> - intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
> - else
> - intel_ucode_patch = p->data;
> - }
> + if (IS_ENABLED(CONFIG_X86_32))
> + intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
> + else
> + intel_ucode_patch = p->data;
> }
>
> static int microcode_sanity_check(void *mc, int print_err)
On Fri, Aug 25, 2017 at 12:40:54PM +0200, walter harms wrote:
> just a bit nitpicking,
> i would expect something like that:
Apply the patch and look at the whole function. Then you'll see why.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Commit-ID: aa78c1ccfab6018289bc2bfd0092d516d0a49ec5
Gitweb: http://git.kernel.org/tip/aa78c1ccfab6018289bc2bfd0092d516d0a49ec5
Author: Borislav Petkov <[email protected]>
AuthorDate: Fri, 25 Aug 2017 12:04:56 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Tue, 29 Aug 2017 10:59:28 +0200
x86/microcode/intel: Improve microcode patches saving flow
Avoid potentially dereferencing a NULL pointer when saving a microcode
patch for early loading on the application processors.
While at it, drop the IS_ERR() checking in favor of simpler, NULL-ptr
checks which are sufficient and rename __alloc_microcode_buf() to
memdup_patch() to more precisely denote what it does.
No functionality change.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/cpu/microcode/intel.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9..8f7a9bb 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -146,18 +146,18 @@ static bool microcode_matches(struct microcode_header_intel *mc_header,
return false;
}
-static struct ucode_patch *__alloc_microcode_buf(void *data, unsigned int size)
+static struct ucode_patch *memdup_patch(void *data, unsigned int size)
{
struct ucode_patch *p;
p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
if (!p)
- return ERR_PTR(-ENOMEM);
+ return NULL;
p->data = kmemdup(data, size, GFP_KERNEL);
if (!p->data) {
kfree(p);
- return ERR_PTR(-ENOMEM);
+ return NULL;
}
return p;
@@ -183,8 +183,8 @@ static void save_microcode_patch(void *data, unsigned int size)
if (mc_hdr->rev <= mc_saved_hdr->rev)
continue;
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer %p\n", data);
else
list_replace(&iter->plist, &p->plist);
@@ -196,24 +196,25 @@ static void save_microcode_patch(void *data, unsigned int size)
* newly found.
*/
if (!prev_found) {
- p = __alloc_microcode_buf(data, size);
- if (IS_ERR(p))
+ p = memdup_patch(data, size);
+ if (!p)
pr_err("Error allocating buffer for %p\n", data);
else
list_add_tail(&p->plist, µcode_cache);
}
+ if (!p)
+ return;
+
/*
* Save for early loading. On 32-bit, that needs to be a physical
* address as the APs are running from physical addresses, before
* paging has been enabled.
*/
- if (p) {
- if (IS_ENABLED(CONFIG_X86_32))
- intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
- else
- intel_ucode_patch = p->data;
- }
+ if (IS_ENABLED(CONFIG_X86_32))
+ intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
+ else
+ intel_ucode_patch = p->data;
}
static int microcode_sanity_check(void *mc, int print_err)