* [PATCH] x86/microcode/intel: Silence a static checker warning
@ 2017-08-22 20:44 Dan Carpenter
2017-08-22 21:13 ` Borislav Petkov
0 siblings, 1 reply; 29+ messages in thread
From: Dan Carpenter @ 2017-08-22 20:44 UTC (permalink / raw)
To: kernel-janitors
Smatch complains that we could dereference a "p" when it's an error
pointer. It's seems unlikely, but it's easy enough to make the checker
happy.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 59edbe9d4ccb..c9c46a138e0e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -208,7 +208,7 @@ static void save_microcode_patch(void *data, unsigned int size)
* address as the APs are running from physical addresses, before
* paging has been enabled.
*/
- if (p) {
+ if (!IS_ERR_OR_NULL(p)) {
if (IS_ENABLED(CONFIG_X86_32))
intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
else
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] x86/microcode/intel: Silence a static checker warning
2017-08-22 20:44 [PATCH] x86/microcode/intel: Silence a static checker warning Dan Carpenter
@ 2017-08-22 21:13 ` Borislav Petkov
2017-08-24 20:15 ` Dan Carpenter
0 siblings, 1 reply; 29+ messages in thread
From: Borislav Petkov @ 2017-08-22 21:13 UTC (permalink / raw)
To: kernel-janitors
On Tue, Aug 22, 2017 at 11:44:54PM +0300, Dan Carpenter wrote:
> Smatch complains
Haha, smatch complains.
> that we could dereference a "p" when it's an error
> pointer. It's seems unlikely, but it's easy enough to make the checker
> happy.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 59edbe9d4ccb..c9c46a138e0e 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -208,7 +208,7 @@ static void save_microcode_patch(void *data, unsigned int size)
> * address as the APs are running from physical addresses, before
> * paging has been enabled.
> */
> - if (p) {
A bit higher in that same function:
if (IS_ERR(p))
pr_err("Error allocating buffer for %p\n", data);
A proper fix would be:
if (IS_ERR_OR_NULL(p)) {
pr_err("Error allocating buffer for %p\n", data);
return;
}
and then drop the if (p) check below.
The same should be done in the above list_for_each_entry_safe() loop.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-22 21:13 ` Borislav Petkov
@ 2017-08-24 20:15 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 20:15 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
---
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);
}
/*
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 20:15 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 20:15 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
---
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);
}
/*
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 20:15 ` Dan Carpenter
@ 2017-08-24 20:47 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 20:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
> ---
> 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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 20:47 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 20:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
> ---
> 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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 20:47 ` Borislav Petkov
@ 2017-08-24 20:55 ` Dan Carpenter
-1 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 20:55 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 20:55 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 20:55 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 20:55 ` Dan Carpenter
@ 2017-08-24 20:58 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 20:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 20:58 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 20:58 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 20:47 ` Borislav Petkov
@ 2017-08-24 21:02 ` Joe Perches
-1 siblings, 0 replies; 29+ messages in thread
From: Joe Perches @ 2017-08-24 21:02 UTC (permalink / raw)
To: Borislav Petkov, Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 21:02 ` Joe Perches
0 siblings, 0 replies; 29+ messages in thread
From: Joe Perches @ 2017-08-24 21:02 UTC (permalink / raw)
To: Borislav Petkov, Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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 <dan.carpenter@oracle.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 20:58 ` Borislav Petkov
@ 2017-08-24 21:08 ` Dan Carpenter
-1 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 21:08 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 21:08 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-24 21:08 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 21:08 ` Dan Carpenter
@ 2017-08-24 21:12 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 21:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-24 21:12 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-24 21:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-24 21:12 ` Borislav Petkov
@ 2017-08-25 9:06 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 9:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-25 9:06 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 9:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-25 9:06 ` Borislav Petkov
@ 2017-08-25 9:12 ` Dan Carpenter
-1 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-25 9:12 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
That patch looks good to me. Could you just add a changelog and give
me a Reported-by: credit?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-25 9:12 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2017-08-25 9:12 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
That patch looks good to me. Could you just add a changelog and give
me a Reported-by: credit?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
2017-08-25 9:12 ` Dan Carpenter
@ 2017-08-25 9:14 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 9:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] x86/microcode: Silence a static checker warning
@ 2017-08-25 9:14 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 9:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] x86/microcode/intel: Improve microcode patches saving flow
2017-08-25 9:14 ` Borislav Petkov
@ 2017-08-25 10:04 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 10:04 UTC (permalink / raw)
To: x86-ml
Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel, kernel-janitors
From: Borislav Petkov <bp@suse.de>
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 <dan.carpenter@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] x86/microcode/intel: Improve microcode patches saving flow
@ 2017-08-25 10:04 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 10:04 UTC (permalink / raw)
To: x86-ml
Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
linux-kernel, kernel-janitors
From: Borislav Petkov <bp@suse.de>
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 <dan.carpenter@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
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.
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] x86/microcode/intel: Improve microcode patches saving flow
2017-08-25 10:04 ` Borislav Petkov
@ 2017-08-25 10:40 ` walter harms
-1 siblings, 0 replies; 29+ messages in thread
From: walter harms @ 2017-08-25 10:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86-ml, Dan Carpenter, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, linux-kernel, kernel-janitors
Am 25.08.2017 12:04, schrieb Borislav Petkov:
> From: Borislav Petkov <bp@suse.de>
>
> 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 <dan.carpenter@oracle.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> 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)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] x86/microcode/intel: Improve microcode patches saving flow
@ 2017-08-25 10:40 ` walter harms
0 siblings, 0 replies; 29+ messages in thread
From: walter harms @ 2017-08-25 10:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86-ml, Dan Carpenter, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, linux-kernel, kernel-janitors
Am 25.08.2017 12:04, schrieb Borislav Petkov:
> From: Borislav Petkov <bp@suse.de>
>
> 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 <dan.carpenter@oracle.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> 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)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] x86/microcode/intel: Improve microcode patches saving flow
2017-08-25 10:40 ` walter harms
@ 2017-08-25 11:41 ` Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 11:41 UTC (permalink / raw)
To: walter harms
Cc: x86-ml, Dan Carpenter, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] x86/microcode/intel: Improve microcode patches saving flow
@ 2017-08-25 11:41 ` Borislav Petkov
0 siblings, 0 replies; 29+ messages in thread
From: Borislav Petkov @ 2017-08-25 11:41 UTC (permalink / raw)
To: walter harms
Cc: x86-ml, Dan Carpenter, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [tip:x86/microcode] x86/microcode/intel: Improve microcode patches saving flow
2017-08-25 10:04 ` Borislav Petkov
(?)
(?)
@ 2017-08-29 9:03 ` tip-bot for Borislav Petkov
-1 siblings, 0 replies; 29+ messages in thread
From: tip-bot for Borislav Petkov @ 2017-08-29 9:03 UTC (permalink / raw)
To: linux-tip-commits; +Cc: hpa, tglx, mingo, linux-kernel, bp, dan.carpenter
Commit-ID: aa78c1ccfab6018289bc2bfd0092d516d0a49ec5
Gitweb: http://git.kernel.org/tip/aa78c1ccfab6018289bc2bfd0092d516d0a49ec5
Author: Borislav Petkov <bp@suse.de>
AuthorDate: Fri, 25 Aug 2017 12:04:56 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
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 <dan.carpenter@oracle.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: kernel-janitors@vger.kernel.org
Link: http://lkml.kernel.org/r/20170825100456.n236w3jebteokfd6@pd.tnic
---
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)
^ permalink raw reply related [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-08-29 9:06 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-22 20:44 [PATCH] x86/microcode/intel: Silence a static checker warning Dan Carpenter
2017-08-22 21:13 ` Borislav Petkov
2017-08-24 20:15 ` [PATCH v2] x86/microcode: " Dan Carpenter
2017-08-24 20:15 ` Dan Carpenter
2017-08-24 20:47 ` Borislav Petkov
2017-08-24 20:47 ` Borislav Petkov
2017-08-24 20:55 ` Dan Carpenter
2017-08-24 20:55 ` Dan Carpenter
2017-08-24 20:58 ` Borislav Petkov
2017-08-24 20:58 ` Borislav Petkov
2017-08-24 21:08 ` Dan Carpenter
2017-08-24 21:08 ` Dan Carpenter
2017-08-24 21:12 ` Borislav Petkov
2017-08-24 21:12 ` Borislav Petkov
2017-08-25 9:06 ` Borislav Petkov
2017-08-25 9:06 ` Borislav Petkov
2017-08-25 9:12 ` Dan Carpenter
2017-08-25 9:12 ` Dan Carpenter
2017-08-25 9:14 ` Borislav Petkov
2017-08-25 9:14 ` Borislav Petkov
2017-08-25 10:04 ` [PATCH] x86/microcode/intel: Improve microcode patches saving flow Borislav Petkov
2017-08-25 10:04 ` Borislav Petkov
2017-08-25 10:40 ` walter harms
2017-08-25 10:40 ` walter harms
2017-08-25 11:41 ` Borislav Petkov
2017-08-25 11:41 ` Borislav Petkov
2017-08-29 9:03 ` [tip:x86/microcode] " tip-bot for Borislav Petkov
2017-08-24 21:02 ` [PATCH v2] x86/microcode: Silence a static checker warning Joe Perches
2017-08-24 21:02 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.