Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Stefan Saecherl <stefan.saecherl@fau.de>
To:     x86@kernel.org
Cc:     linux-kernel@i4.cs.fau.de,
        Stefan Saecherl <stefan.saecherl@fau.de>,
        Lorena Kretzschmar <qy15sije@cip.cs.fau.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Alexandre Chartre <alexandre.chartre@oracle.com>,
        Mike Rapoport <rppt@kernel.org>,
        Ira Weiny <ira.weiny@intel.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: [PATCH] x86/kgdb: Allow removal of early BPs
Date:   Mon, 14 Dec 2020 15:13:12 +0100
Message-Id: <20201214141314.5717-1-stefan.saecherl@fau.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

The problem is that breakpoints that are set early (e.g. via kgdbwait)
cannot be deleted after boot completed (to be precise after mark_rodata_ro
ran).

When setting a breakpoint early there are executable pages that are
writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint
succeeds and the breakpoint is saved as type BP_BREAKPOINT.

Later in the boot write access to these pages is restricted. So when
removing the breakpoint the copy_to_kernel_nofault call in
kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal
fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which
can work around nonwriteability.

One thing to consider when doing this is that code can go away during boot
(e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case
gracefully by just having copy_to_kernel_nofault fail but if one then calls
text_poke_kgdb the system dies due to the BUG_ON we moved out of
__text_poke.  To avoid this __text_poke now returns an error in case of a
nonpresent code page and the error is handled at call site.

Checkpatch complains about two uses of BUG_ON but the new code should not
trigger BUG_ON in cases where the old didn't.

Co-developed-by: Lorena Kretzschmar <qy15sije@cip.cs.fau.de>
Signed-off-by: Lorena Kretzschmar <qy15sije@cip.cs.fau.de>
Signed-off-by: Stefan Saecherl <stefan.saecherl@fau.de>
---
 arch/x86/kernel/alternative.c | 16 +++++++----
 arch/x86/kernel/kgdb.c        | 54 ++++++++++++++++++++++++-----------
 2 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 2400ad62f330..0f145d837885 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
 		if (cross_page_boundary)
 			pages[1] = virt_to_page(addr + PAGE_SIZE);
 	}
-	/*
-	 * If something went wrong, crash and burn since recovery paths are not
-	 * implemented.
-	 */
-	BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
+
+	if (!pages[0] || (cross_page_boundary && !pages[1]))
+		return ERR_PTR(-EFAULT);
 
 	/*
 	 * Map the page without the global bit, as TLB flushing is done with
@@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t len)
 {
 	lockdep_assert_held(&text_mutex);
 
-	return __text_poke(addr, opcode, len);
+	addr = __text_poke(addr, opcode, len);
+	/*
+	 * If something went wrong, crash and burn since recovery paths are not
+	 * implemented.
+	 */
+	BUG_ON(IS_ERR(addr));
+	return addr;
 }
 
 /**
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index ff7878df96b4..e98c9c43db7c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 {
 	int err;
+	void *addr;
 
 	bpt->type = BP_BREAKPOINT;
 	err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
@@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 	 */
 	if (mutex_is_locked(&text_mutex))
 		return -EBUSY;
-	text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
-		       BREAK_INSTR_SIZE);
+
+	addr = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
+				BREAK_INSTR_SIZE);
+	/* This should never trigger because the above call to copy_from_kernel_nofault
+	 * already succeeded.
+	 */
+	BUG_ON(IS_ERR(addr));
+
 	bpt->type = BP_POKE_BREAKPOINT;
 
 	return 0;
@@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
 
 int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 {
-	if (bpt->type != BP_POKE_BREAKPOINT)
-		goto knl_write;
-	/*
-	 * It is safe to call text_poke_kgdb() because normal kernel execution
-	 * is stopped on all cores, so long as the text_mutex is not locked.
-	 */
-	if (mutex_is_locked(&text_mutex))
-		goto knl_write;
-	text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
-		       BREAK_INSTR_SIZE);
-	return 0;
+	void *addr;
+	int err;
 
-knl_write:
-	return copy_to_kernel_nofault((char *)bpt->bpt_addr,
-				  (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
+	if (bpt->type == BP_POKE_BREAKPOINT) {
+		if (mutex_is_locked(&text_mutex)) {
+			err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
+							(char *)bpt->saved_instr,
+							BREAK_INSTR_SIZE);
+		} else {
+			/*
+			 * It is safe to call text_poke_kgdb() because normal kernel execution
+			 * is stopped on all cores, so long as the text_mutex is not locked.
+			 */
+			addr = text_poke_kgdb((void *)bpt->bpt_addr,
+							bpt->saved_instr,
+							BREAK_INSTR_SIZE);
+			err = PTR_ERR_OR_ZERO(addr);
+		}
+	} else {
+		err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
+						(char *)bpt->saved_instr,
+						BREAK_INSTR_SIZE);
+		if (err == -EFAULT && !mutex_is_locked(&text_mutex)) {
+			addr = text_poke_kgdb((void *)bpt->bpt_addr,
+						bpt->saved_instr,
+						BREAK_INSTR_SIZE);
+			err = PTR_ERR_OR_ZERO(addr);
+		}
+	}
+	return err;
 }
 
 const struct kgdb_arch arch_kgdb_ops = {
-- 
2.20.1