From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on archive.lwn.net X-Spam-Level: X-Spam-Status: No, score=-5.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=unavailable autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by archive.lwn.net (Postfix) with ESMTP id 801567D043 for ; Thu, 7 Jun 2018 16:27:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751157AbeFGQ04 (ORCPT ); Thu, 7 Jun 2018 12:26:56 -0400 Received: from mga01.intel.com ([192.55.52.88]:23692 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753833AbeFGQ0z (ORCPT ); Thu, 7 Jun 2018 12:26:55 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Jun 2018 09:26:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,487,1520924400"; d="scan'208";a="65161441" Received: from 2b52.sc.intel.com (HELO [143.183.136.51]) ([143.183.136.51]) by orsmga002.jf.intel.com with ESMTP; 07 Jun 2018 09:26:53 -0700 Message-ID: <1528388623.4636.19.camel@2b52.sc.intel.com> Subject: Re: [PATCH 1/9] x86/cet: Control protection exception handler From: Yu-cheng Yu To: Andy Lutomirski Cc: LKML , linux-doc@vger.kernel.org, Linux-MM , linux-arch , X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , "H. J. Lu" , "Shanbhogue, Vedvyas" , "Ravi V. Shankar" , Dave Hansen , Jonathan Corbet , Oleg Nesterov , Arnd Bergmann , mike.kravetz@oracle.com Date: Thu, 07 Jun 2018 09:23:43 -0700 In-Reply-To: References: <20180607143705.3531-1-yu-cheng.yu@intel.com> <20180607143705.3531-2-yu-cheng.yu@intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org On Thu, 2018-06-07 at 08:46 -0700, Andy Lutomirski wrote: > On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu wrote: > > ... > > diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S > > index bef8e2b202a8..14b63ef0d7d8 100644 > > --- a/arch/x86/entry/entry_32.S > > +++ b/arch/x86/entry/entry_32.S > > @@ -1070,6 +1070,11 @@ ENTRY(general_protection) > > jmp common_exception > > END(general_protection) > > > > +ENTRY(control_protection) > > + pushl $do_control_protection > > + jmp common_exception > > +END(control_protection) > > Ugh, you're seriously supporting this on 32-bit? Please test double > fault handling very carefully -- the CET interaction with task > switches is so gross that I didn't even bother reading the spec except > to let the architects know that they were a but nuts to support it at > all. > I will remove this. ... > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index 03f3d7695dac..4e8769a19aaf 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > > +/* > > + * When a control protection exception occurs, send a signal > > + * to the responsible application. Currently, control > > + * protection is only enabled for the user mode. This > > + * exception should not come from the kernel mode. > > + */ > > +dotraplinkage void > > +do_control_protection(struct pt_regs *regs, long error_code) > > +{ > > + struct task_struct *tsk; > > + > > + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU"); > > + cond_local_irq_enable(regs); > > + > > + tsk = current; > > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) && > > + !cpu_feature_enabled(X86_FEATURE_IBT)) { > > static_cpu_has(), please. But your handling here is odd -- I think > that we should at least warn if we get #CP with CET disable. I will fix it. > > > + goto exit; > > + } > > + > > + if (!user_mode(regs)) { > > + tsk->thread.error_code = error_code; > > + tsk->thread.trap_nr = X86_TRAP_CP; > > I realize you copied this from elsewhere in the file, but please > either delete these assignments to error_code and trap_nr or at least > hoist them out of the if block. I will fix it. > > > + if (notify_die(DIE_TRAP, "control protection fault", regs, > > + error_code, X86_TRAP_CP, SIGSEGV) != NOTIFY_STOP) > > Does this notify_die() check serve any purpose at all? Removing all > the old ones would be a project, but let's try not to add new callers. OK. > > > + die("control protection fault", regs, error_code); > > + return; > > + } > > + > > + tsk->thread.error_code = error_code; > > + tsk->thread.trap_nr = X86_TRAP_CP; > > + > > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) && > > + printk_ratelimit()) { > > + unsigned int max_idx, err_idx; > > + > > + max_idx = ARRAY_SIZE(control_protection_err) - 1; > > + err_idx = min((unsigned int)error_code - 1, max_idx); > > What if error_code == 0? Is that also invalid? The error code is between 1 and 5 inclusive. I thought if it is 0, then err_idx would become max_idx here. I can change it to: if (error_code == 0) error_code = max_idx; Or, add some comments for this case. > > > + pr_info("%s[%d] control protection ip:%lx sp:%lx error:%lx(%s)", > > + tsk->comm, task_pid_nr(tsk), > > + regs->ip, regs->sp, error_code, > > + control_protection_err[err_idx]); > > + print_vma_addr(" in ", regs->ip); > > + pr_cont("\n"); > > + } > > + > > +exit: > > + force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk); > > This is definitely wrong for the feature-disabled, !user_mode case. > I will fix it. > Also, are you planning on enabling CET for kernel code too? Yes, kernel protection will be enabled later. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html