Re: [PATCH 12/13] x86/jitalloc: prepare to allocate exectuatble memory as ROX

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"rppt@kernel.org" <rppt@kernel.org>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"deller@gmx.de" <deller@gmx.de>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"linux-mips@vger.kernel.org" <linux-mips@vger.kernel.org>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"hca@linux.ibm.com" <hca@linux.ibm.com>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"kent.overstreet@linux.dev" <kent.overstreet@linux.dev>,
	"linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"christophe.leroy@csgroup.eu" <christophe.leroy@csgroup.eu>,
	"chenhuacai@kernel.org" <chenhuacai@kernel.org>,
	"mpe@ellerman.id.au" <mpe@ellerman.id.au>,
	"linux-trace-kernel@vger.kernel.org"
	<linux-trace-kernel@vger.kernel.org>,
	"tsbogend@alpha.franken.de" <tsbogend@alpha.franken.de>,
	"palmer@dabbelt.com" <palmer@dabbelt.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-parisc@vger.kernel.org" <linux-parisc@vger.kernel.org>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"will@kernel.org" <will@kernel.org>,
	"dinguyen@kernel.org" <dinguyen@kernel.org>,
	"naveen.n.rao@linux.ibm.com" <naveen.n.rao@linux.ibm.com>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	"linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"song@kernel.org" <song@kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"loongarch@lists.linux.dev" <loongarch@lists.linux.dev>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>
Subject: Re: [PATCH 12/13] x86/jitalloc: prepare to allocate exectuatble memory as ROX
Date: Thu, 1 Jun 2023 16:54:27 +0000	[thread overview]
Message-ID: <0f50ac52a5280d924beeb131e6e4717b6ad9fdf7.camel@intel.com> (raw)
In-Reply-To: <20230601101257.530867-13-rppt@kernel.org>

On Thu, 2023-06-01 at 13:12 +0300, Mike Rapoport wrote:
>  /*
>   * Are we looking at a near JMP with a 1 or 4-byte displacement.
> @@ -331,7 +344,7 @@ void __init_or_module noinline
> apply_alternatives(struct alt_instr *start,
>  
>                 DUMP_BYTES(insn_buff, insn_buff_sz, "%px: final_insn:
> ", instr);
>  
> -               text_poke_early(instr, insn_buff, insn_buff_sz);
> +               do_text_poke(instr, insn_buff, insn_buff_sz);
>  
>  next:
>                 optimize_nops(instr, a->instrlen);
> @@ -564,7 +577,7 @@ void __init_or_module noinline
> apply_retpolines(s32 *start, s32 *end)
>                         optimize_nops(bytes, len);
>                         DUMP_BYTES(((u8*)addr),  len, "%px: orig: ",
> addr);
>                         DUMP_BYTES(((u8*)bytes), len, "%px: repl: ",
> addr);
> -                       text_poke_early(addr, bytes, len);
> +                       do_text_poke(addr, bytes, len);
>                 }
>         }
>  }
> @@ -638,7 +651,7 @@ void __init_or_module noinline apply_returns(s32
> *start, s32 *end)
>                 if (len == insn.length) {
>                         DUMP_BYTES(((u8*)addr),  len, "%px: orig: ",
> addr);
>                         DUMP_BYTES(((u8*)bytes), len, "%px: repl: ",
> addr);
> -                       text_poke_early(addr, bytes, len);
> +                       do_text_poke(addr, bytes, len);
>                 }
>         }
>  }
> @@ -674,7 +687,7 @@ static void poison_endbr(void *addr, bool warn)
>          */
>         DUMP_BYTES(((u8*)addr), 4, "%px: orig: ", addr);
>         DUMP_BYTES(((u8*)&poison), 4, "%px: repl: ", addr);
> -       text_poke_early(addr, &poison, 4);
> +       do_text_poke(addr, &poison, 4);
>  }
>  
>  /*
> @@ -869,7 +882,7 @@ static int cfi_disable_callers(s32 *start, s32
> *end)
>                 if (!hash) /* nocfi callers */
>                         continue;
>  
> -               text_poke_early(addr, jmp, 2);
> +               do_text_poke(addr, jmp, 2);
>         }
>  
>         return 0;
> @@ -892,7 +905,7 @@ static int cfi_enable_callers(s32 *start, s32
> *end)
>                 if (!hash) /* nocfi callers */
>                         continue;
>  
> -               text_poke_early(addr, mov, 2);
> +               do_text_poke(addr, mov, 2);
>         }
>  
>         return 0;
> @@ -913,7 +926,7 @@ static int cfi_rand_preamble(s32 *start, s32
> *end)
>                         return -EINVAL;
>  
>                 hash = cfi_rehash(hash);
> -               text_poke_early(addr + 1, &hash, 4);
> +               do_text_poke(addr + 1, &hash, 4);
>         }
>  
>         return 0;
> @@ -932,9 +945,9 @@ static int cfi_rewrite_preamble(s32 *start, s32
> *end)
>                          addr, addr, 5, addr))
>                         return -EINVAL;
>  
> -               text_poke_early(addr, fineibt_preamble_start,
> fineibt_preamble_size);
> +               do_text_poke(addr, fineibt_preamble_start,
> fineibt_preamble_size);
>                 WARN_ON(*(u32 *)(addr + fineibt_preamble_hash) !=
> 0x12345678);
> -               text_poke_early(addr + fineibt_preamble_hash, &hash,
> 4);
> +               do_text_poke(addr + fineibt_preamble_hash, &hash, 4);
>         }

It is just a local flush, but I wonder how much text_poke()ing is too
much. A lot of the are even inside loops. Can't it do the batch version
at least?

The other thing, and maybe this is in paranoia category, but it's
probably at least worth noting. Before the modules were not made
executable until all of the code was finalized. Now they are made
executable in an intermediate state and then patched later. It might
weaken the CFI stuff, but also it just kind of seems a bit unbounded
for dealing with executable code.

Preparing the modules in a separate RW mapping, and then text_poke()ing
the whole thing in when you are done would resolve both of these.