* [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: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
* 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
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.