* [bug report] x86/mce: Prevent severity computation from being instrumented
@ 2021-12-17 10:20 Dan Carpenter
2021-12-17 11:29 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-12-17 10:20 UTC (permalink / raw)
To: bp; +Cc: linux-edac
Hello Borislav Petkov,
This is a semi-automatic email about new static checker warnings.
The patch 0a5b288e85bb: "x86/mce: Prevent severity computation from
being instrumented" from Oct 13, 2021, leads to the following Smatch
complaint:
arch/x86/kernel/cpu/mce/severity.c:286 error_context()
warn: variable dereferenced before check 'regs' (see line 280)
arch/x86/kernel/cpu/mce/severity.c
279 fixup_type = ex_get_fixup_type(m->ip);
280 copy_user = is_copy_from_user(regs);
^^^^
New unchecked dereference
281 instrumentation_end();
282
283 switch (fixup_type) {
284 case EX_TYPE_UACCESS:
285 case EX_TYPE_COPY:
286 if (!regs || !copy_user)
^^^^
Old code checked for NULL
287 return IN_KERNEL;
288 m->kflags |= MCE_IN_KERNEL_COPYIN;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] x86/mce: Prevent severity computation from being instrumented 2021-12-17 10:20 [bug report] x86/mce: Prevent severity computation from being instrumented Dan Carpenter @ 2021-12-17 11:29 ` Borislav Petkov 2021-12-17 17:13 ` Luck, Tony 0 siblings, 1 reply; 4+ messages in thread From: Borislav Petkov @ 2021-12-17 11:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-edac, Tony Luck + Tony. On Fri, Dec 17, 2021 at 01:20:29PM +0300, Dan Carpenter wrote: > Hello Borislav Petkov, > > This is a semi-automatic email about new static checker warnings. > > The patch 0a5b288e85bb: "x86/mce: Prevent severity computation from > being instrumented" from Oct 13, 2021, leads to the following Smatch > complaint: > > arch/x86/kernel/cpu/mce/severity.c:286 error_context() > warn: variable dereferenced before check 'regs' (see line 280) > > arch/x86/kernel/cpu/mce/severity.c > 279 fixup_type = ex_get_fixup_type(m->ip); > 280 copy_user = is_copy_from_user(regs); > ^^^^ > New unchecked dereference > > 281 instrumentation_end(); > 282 > 283 switch (fixup_type) { > 284 case EX_TYPE_UACCESS: > 285 case EX_TYPE_COPY: > 286 if (!regs || !copy_user) > ^^^^ > Old code checked for NULL > > 287 return IN_KERNEL; > 288 m->kflags |= MCE_IN_KERNEL_COPYIN; Good catch, thanks! I guess we can do something like this: --- diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c index a32646769705..7aa2bda93cbb 100644 --- a/arch/x86/kernel/cpu/mce/severity.c +++ b/arch/x86/kernel/cpu/mce/severity.c @@ -222,6 +222,9 @@ static bool is_copy_from_user(struct pt_regs *regs) struct insn insn; int ret; + if (!regs) + return false; + if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip, MAX_INSN_SIZE)) return false; @@ -283,7 +286,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs) switch (fixup_type) { case EX_TYPE_UACCESS: case EX_TYPE_COPY: - if (!regs || !copy_user) + if (!copy_user) return IN_KERNEL; m->kflags |= MCE_IN_KERNEL_COPYIN; fallthrough; -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [bug report] x86/mce: Prevent severity computation from being instrumented 2021-12-17 11:29 ` Borislav Petkov @ 2021-12-17 17:13 ` Luck, Tony 2021-12-17 23:50 ` [PATCH] x86/mce: Check regs before accessing it Borislav Petkov 0 siblings, 1 reply; 4+ messages in thread From: Luck, Tony @ 2021-12-17 17:13 UTC (permalink / raw) To: Borislav Petkov; +Cc: Dan Carpenter, linux-edac On Fri, Dec 17, 2021 at 12:29:45PM +0100, Borislav Petkov wrote: > + Tony. > > On Fri, Dec 17, 2021 at 01:20:29PM +0300, Dan Carpenter wrote: > > Hello Borislav Petkov, > > > > This is a semi-automatic email about new static checker warnings. > > > > The patch 0a5b288e85bb: "x86/mce: Prevent severity computation from > > being instrumented" from Oct 13, 2021, leads to the following Smatch > > complaint: > > > > arch/x86/kernel/cpu/mce/severity.c:286 error_context() > > warn: variable dereferenced before check 'regs' (see line 280) > > > > arch/x86/kernel/cpu/mce/severity.c > > 279 fixup_type = ex_get_fixup_type(m->ip); > > 280 copy_user = is_copy_from_user(regs); > > ^^^^ > > New unchecked dereference > > > > 281 instrumentation_end(); > > 282 > > 283 switch (fixup_type) { > > 284 case EX_TYPE_UACCESS: > > 285 case EX_TYPE_COPY: > > 286 if (!regs || !copy_user) > > ^^^^ > > Old code checked for NULL > > > > 287 return IN_KERNEL; > > 288 m->kflags |= MCE_IN_KERNEL_COPYIN; > > Good catch, thanks! > > I guess we can do something like this: > > --- > diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c > index a32646769705..7aa2bda93cbb 100644 > --- a/arch/x86/kernel/cpu/mce/severity.c > +++ b/arch/x86/kernel/cpu/mce/severity.c > @@ -222,6 +222,9 @@ static bool is_copy_from_user(struct pt_regs *regs) > struct insn insn; > int ret; > > + if (!regs) > + return false; > + > if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip, MAX_INSN_SIZE)) > return false; > > @@ -283,7 +286,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs) > switch (fixup_type) { > case EX_TYPE_UACCESS: > case EX_TYPE_COPY: > - if (!regs || !copy_user) > + if (!copy_user) > return IN_KERNEL; > m->kflags |= MCE_IN_KERNEL_COPYIN; > fallthrough; > Looks good to me. Reviewed-by: Tony Luck <tony.luck@intel.com> -Tony ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] x86/mce: Check regs before accessing it 2021-12-17 17:13 ` Luck, Tony @ 2021-12-17 23:50 ` Borislav Petkov 0 siblings, 0 replies; 4+ messages in thread From: Borislav Petkov @ 2021-12-17 23:50 UTC (permalink / raw) To: Luck, Tony; +Cc: Dan Carpenter, linux-edac, lkml From: Borislav Petkov <bp@suse.de> Commit in Fixes accesses pt_regs before checking whether it is NULL or not. Change the flow so that the NULL pointer check happens first. Fixes: 0a5b288e85bb ("x86/mce: Prevent severity computation from being instrumented") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Borislav Petkov <bp@suse.de> Reviewed-by: Tony Luck <tony.luck@intel.com> Link: https://lore.kernel.org/r/20211217102029.GA29708@kili --- arch/x86/kernel/cpu/mce/severity.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c index a32646769705..7aa2bda93cbb 100644 --- a/arch/x86/kernel/cpu/mce/severity.c +++ b/arch/x86/kernel/cpu/mce/severity.c @@ -222,6 +222,9 @@ static bool is_copy_from_user(struct pt_regs *regs) struct insn insn; int ret; + if (!regs) + return false; + if (copy_from_kernel_nofault(insn_buf, (void *)regs->ip, MAX_INSN_SIZE)) return false; @@ -283,7 +286,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs) switch (fixup_type) { case EX_TYPE_UACCESS: case EX_TYPE_COPY: - if (!regs || !copy_user) + if (!copy_user) return IN_KERNEL; m->kflags |= MCE_IN_KERNEL_COPYIN; fallthrough; -- 2.29.2 -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-17 23:50 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-17 10:20 [bug report] x86/mce: Prevent severity computation from being instrumented Dan Carpenter 2021-12-17 11:29 ` Borislav Petkov 2021-12-17 17:13 ` Luck, Tony 2021-12-17 23:50 ` [PATCH] x86/mce: Check regs before accessing it Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox