Re: Fix issue with alternatives/paravirt patches

[Jessica Yu]

+++ Miroslav Benes [12/07/16 14:06 +0200]:

On Tue, 5 Jul 2016, Jessica Yu wrote:


Hi,

A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.

One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.

To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.

Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).

I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.

Based on linux-next.

[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580

Jessica Yu (2):
  livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
  livepatch/x86: apply alternatives and paravirt patches after relocations

 arch/x86/kernel/Makefile|  1 +
 arch/x86/kernel/livepatch.c | 66 +
 include/linux/livepatch.h   |  3 +++
 kernel/livepatch/core.c | 12 +++--
 4 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/kernel/livepatch.c


Hi,

thanks Jessica for implementing it. It does not look as bad as I was
afraid, which is great. Few remarks...

Is there a problem when you need to generate a dynrela for paravirt code?
I mean one does not know during the build of a patch module if paravirt
would or would not be applied. If such code needs to be relocated it could
be a problem for kpatch-build. Is this correct or am I missing something?


In kpatch-build, "special" sections like .parainstructions and
.altinstructions are into consideration. These sections are included
in the final patch module if they reference any of the new replacement
code. Like with any other section, kpatch-build will convert any relas
from .rela.parainstructions (for example) to dynrelas if it is
necessary.  And with this patch we apply all "dynrelas" (which are now
actual relas in .klp.rela sections) first before applying any
paravirt/alternatives patches in arch_klp_init_object_loaded(), which
is the correct order.


Now the other architectures we support. s390 should be ok. There is
nothing much in module_finalize() there. powerpc is different though. It
is quite similar to x86_64 case. And also arm64 needs to be handled in
future.


Yeah, I think we are OK for s390, arm64 looks fairly straightforward
(just a call to apply_alternatives()), powerpc looks slightly more
involved but I haven't thoroughly dug into it yet.

So other arches will need to have arch_klp_init_object_loaded()
implemented eventually (if needed), but I think it is fine for now to
fix this issue on x86 first, since that's where the original bug
cropped up.

Jessica

[PATCH v2 1/2] livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks

[Jessica Yu]

Introduce arch_klp_init_object_loaded() to complete any additional
arch-specific tasks during patching. Architecture code may override this
function.

Signed-off-by: Jessica Yu 
---
 include/linux/livepatch.h |  3 +++
 kernel/livepatch/core.c   | 12 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index a93a0b2..9072f04 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -116,6 +116,9 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+struct klp_object *obj);
+
 /* Called from the module loader during module coming/going states */
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5c2bc10..164eff6 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -274,7 +274,6 @@ static int klp_write_object_relocations(struct module *pmod,
 
objname = klp_is_module(obj) ? obj->name : "vmlinux";
 
-   module_disable_ro(pmod);
/* For each klp relocation section */
for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
sec = pmod->klp_info->sechdrs + i;
@@ -309,7 +308,6 @@ static int klp_write_object_relocations(struct module *pmod,
break;
}
 
-   module_enable_ro(pmod);
return ret;
 }
 
@@ -763,6 +761,12 @@ static int klp_init_func(struct klp_object *obj, struct 
klp_func *func)
func->old_sympos ? func->old_sympos : 1);
 }
 
+/* Arches may override this to finish any remaining arch-specific tasks */
+void __weak arch_klp_init_object_loaded(struct klp_patch *patch,
+   struct klp_object *obj)
+{
+}
+
 /* parts of the initialization that is done only when the object is loaded */
 static int klp_init_object_loaded(struct klp_patch *patch,
  struct klp_object *obj)
@@ -770,10 +774,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
struct klp_func *func;
int ret;
 
+   module_disable_ro(patch->mod);
ret = klp_write_object_relocations(patch->mod, obj);
if (ret)
return ret;
 
+   arch_klp_init_object_loaded(patch, obj);
+   module_enable_ro(patch->mod);
+
klp_for_each_func(obj, func) {
ret = klp_find_object_symbol(obj->name, func->old_name,
 func->old_sympos,
-- 
2.5.5

[PATCH v2 2/2] livepatch/x86: apply alternatives and paravirt patches after relocations

[Jessica Yu]

Implement arch_klp_init_object_loaded() for x86, which applies
alternatives/paravirt patches. This fixes the order in which relocations
and alternatives/paravirt patches are applied.

Previously, if a patch module had alternatives or paravirt patches,
these were applied first by the module loader before livepatch can apply
per-object relocations. The (buggy) sequence of events was:

(1) Load patch module
(2) Apply alternatives and paravirt patches to patch module
* Note that these are applied to the new functions in the patch module
(3) Apply per-object relocations to patch module when target module loads.
* This clobbers what was written in step 2

This lead to crashes and corruption in general, since livepatch would
overwrite or step on previously applied alternative/paravirt patches.
The correct sequence of events should be:

(1) Load patch module
(2) Apply per-object relocations to patch module
(3) Apply alternatives and paravirt patches to patch module

This is fixed by delaying paravirt/alternatives patching until after
relocations are applied. Any .altinstructions or .parainstructions sections in
a patch module are prefixed with ".klp.arch.${objname}" and applied in
arch_klp_init_object_loaded().

Signed-off-by: Jessica Yu 
---
 arch/x86/kernel/Makefile|  1 +
 arch/x86/kernel/livepatch.c | 65 +
 2 files changed, 66 insertions(+)
 create mode 100644 arch/x86/kernel/livepatch.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 0503f5b..4f656fe 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_X86_MPPARSE) += mpparse.o
 obj-y  += apic/
 obj-$(CONFIG_X86_REBOOTFIXUPS) += reboot_fixups_32.o
 obj-$(CONFIG_DYNAMIC_FTRACE)   += ftrace.o
+obj-$(CONFIG_LIVEPATCH)+= livepatch.o
 obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
 obj-$(CONFIG_FTRACE_SYSCALLS)  += ftrace.o
 obj-$(CONFIG_X86_TSC)  += trace_clock.o
diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
new file mode 100644
index 000..78209f1
--- /dev/null
+++ b/arch/x86/kernel/livepatch.c
@@ -0,0 +1,65 @@
+/*
+ * livepatch.c - x86-specific Kernel Live Patching Core
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+/* Apply per-object alternatives. Based on x86 module_finalize() */
+void arch_klp_init_object_loaded(struct klp_patch *patch,
+struct klp_object *obj)
+{
+   int cnt;
+   struct klp_modinfo *info;
+   Elf_Shdr *s, *alt = NULL, *para = NULL;
+   void *aseg, *pseg;
+   const char *objname;
+   char sec_objname[MODULE_NAME_LEN];
+   char secname[KSYM_NAME_LEN];
+
+   info = patch->mod->klp_info;
+   objname = obj->name ? obj->name : "vmlinux";
+
+   /* See livepatch core code for BUILD_BUG_ON() explanation */
+   BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128);
+
+   for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) {
+   /* Apply per-object .klp.arch sections */
+   cnt = sscanf(info->secstrings + s->sh_name,
+".klp.arch.%55[^.].%127s",
+sec_objname, secname);
+   if (cnt != 2)
+   continue;
+   if (strcmp(sec_objname, objname))
+   continue;
+   if (!strcmp("altinstructions", secname))
+   alt = s;
+   if (!strcmp("parainstructions", secname))
+   para = s;
+   }
+
+   if (alt) {
+   aseg = (void *) alt->sh_addr;
+   apply_alternatives(aseg, aseg + alt->sh_size);
+   }
+
+   if (para) {
+   pseg = (void *) para->sh_addr;
+   apply_paravirt(pseg, pseg + para->sh_size);
+   }
+}
-- 
2.5.5

[PATCH v2 0/2] Fix issue with alternatives/paravirt patches

[Jessica Yu]

Hi,

A few months ago, Chris Arges reported a bug involving alternatives/paravirt
patching that was discussed here [1] and here [2]. To briefly summarize the
bug, patch modules that contained .altinstructions or .parainstructions
sections would break because these alternative/paravirt patches would be
applied first by the module loader (see x86 module_finalize()), then
livepatch would later clobber these patches when applying per-object
relocations. This lead to crashes and unpredictable behavior.

One conclusion we reached from our last discussion was that we will
need to introduce some arch-specific code to address this problem.
This patchset presents a possible fix for the bug by adding a new
arch-specific arch_klp_init_object_loaded() function that by default
does nothing but can be overridden by different arches.

To fix this issue for x86, since we can access a patch module's Elf
sections through mod->klp_info, we can simply delay the calls to
apply_paravirt() and apply_alternatives() to arch_klp_init_object_loaded(),
which is called after relocations have been written for an object.
In addition, for patch modules, .parainstructions and .altinstructions are
prefixed by ".klp.arch.${objname}" so that the module loader ignores them
and livepatch can apply them manually.

Currently for kpatch, we don't support including jump table sections in
the patch module, and supporting .smp_locks is currently broken, so we
don't consider those sections (for now).

I did some light testing with some patches to kvm and verified that the
original issue reported in [2] was fixed.

Based on linux-next.

v1 here:
http://lkml.kernel.org/g/1467772500-26092-1-git-send-email-j...@redhat.com

v2:
 - add BUILD_BUG_ON() check in arch_klp_init_object_loaded (x86)

[1] http://thread.gmane.org/gmane.linux.kernel/2185604/
[2] https://github.com/dynup/kpatch/issues/580

Jessica Yu (2):
  livepatch: use arch_klp_init_object_loaded() to finish arch-specific tasks
  livepatch/x86: apply alternatives and paravirt patches after relocations

 arch/x86/kernel/Makefile|  1 +
 arch/x86/kernel/livepatch.c | 65 +
 include/linux/livepatch.h   |  3 +++
 kernel/livepatch/core.c | 12 +++--
 4 files changed, 79 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/kernel/livepatch.c

-- 
2.5.5

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Kees Cook [21/07/16 16:03 -0700]:

On Wed, Jun 29, 2016 at 9:56 PM, Rusty Russell  wrote:

Jessica Yu  writes:

+++ Rusty Russell [29/06/16 10:38 +0930]:

Jessica Yu  writes:

Add ro_after_init support for modules by adding a new page-aligned section
in the module layout (after rodata) for ro_after_init data and enabling RO
protection for that section after module init runs.

Signed-off-by: Jessica Yu 


I would prefer a "bool after_init" flag to module_enable_ro().  It's
more explicit.


Sure thing, I was just initially worried about the
module_{enable,disable}_ro() asymmetry. :)


Yes, but I think compile-time-analyzable behaviour beats
runtime-analyzable behaviour for clarity.


Exposing the flags via uapi looks like a wart, but it's kind of a
feature, since we don't *unset* it in any section; userspace may want to
know about it.


Hm, I'm still unsure about this. I'm starting to think it might be a
bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
would technically be used only internally in the kernel (i.e. module
loader), and it'd also be considered a non-standard flag, using a bit
from SHF_MASKOS (OS-specific range). What do you think?


Some arch *could* use it by setting the flag in a section in their
module I think; we don't stop them.  Since the other flags are there,
I'd leave it.

We don't expose the flags via sysfs, though, so that's the only
exposure.


What's the state of this series? I'd love it if the functionality
could land for v4.8...



Hi Kees,

Sorry for the delay! Have been busier than usual lately. I'll be able
to get v2 out tomorrow.

Thanks!
Jessica

Re: taint/module: Clean up global and module taint flags handling

[Jessica Yu]

+++ Petr Mladek [21/09/16 13:47 +0200]:

The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
potentially print one more character. But it did not increase the
size of the corresponding buffers in m_show() and print_modules().

We have recently done the same mistake when adding a taint flag
for livepatching, see
https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoim...@redhat.com

Also struct module uses an incompatible type for mod-taints flags.
It survived from the commit 2bc2d61a9638dab670d ("[PATCH] list module
taint flags in Oops/panic"). There was used "int" for the global taint
flags at these times. But only the global tain flags was later changed
to "unsigned long" by the commit 25ddbb18aae33ad2 ("Make the taint
flags reliable").

This patch defines TAINT_FLAGS_COUNT that can be used to create
arrays and buffers of the right size. Note that we could not use
enum because the taint flag indexes are used also in assembly code.

Then it reworks the table that describes the taint flags. The TAINT_*
numbers can be used as the index. Instead, we add information
if the taint flag is also shown per-module.

Finally, it uses "unsigned long", bit operations, and the updated
taint_flags table also for mod->taints.

It is not optimal because only few taint flags can be printed by
module_taint_flags(). But better be on the safe side. IMHO, it is
not worth the optimization and this is a good compromise.

Signed-off-by: Petr Mladek 
---
Changes against v2:

 + fixed a typo in a comment

 + rebased on top of for-next branch from git/jikos/livepatching.git
   that has the commit commit 2992ef29ae01af9983 ("livepatch/module:
    make TAINT_LIVEPATCH module-specific").


Reviewed-by: Jessica Yu 

Hm, quick question, which tree would this patch go to? Though the
cleanup is for modules, there is an indirect cross-tree dependency
(taint_flag.module needs to be true for TAINT_LIVEPATCH for Josh's
patch to still work as intended). The least complicated thing to do
would be to just take this through the livepatch tree (with Rusty's
approval :-)), no?

Re: module/taint: Automatically increase the buffer size for new taint flags

[Jessica Yu]

+++ Petr Mladek [07/09/16 15:13 +0200]:

The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
potentially print one more character. But it did not increase the
size of the corresponding buffers in m_show() and print_modules().

We have recently done the same mistake when adding a taint flag
for livepatching, see
https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoim...@redhat.com

Let's convert the taint flags into enum and handle the buffer size
almost automatically.

It is not optimal because only few taint flags can be printed by
module_taint_flags(). But better be on the safe side. IMHO, it is
not worth the optimization and this is a good compromise.

Signed-off-by: Petr Mladek 
---
include/linux/kernel.h | 44 
kernel/module.c|  8 ++--
kernel/panic.c |  4 ++--
3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d96a6118d26a..1809bc82b7a5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -472,14 +472,10 @@ static inline void set_arch_panic_timeout(int timeout, 
int arch_default_timeout)
if (panic_timeout == arch_default_timeout)
panic_timeout = timeout;
}
-extern const char *print_tainted(void);
enum lockdep_ok {
LOCKDEP_STILL_OK,
LOCKDEP_NOW_UNRELIABLE
};
-extern void add_taint(unsigned flag, enum lockdep_ok);
-extern int test_taint(unsigned flag);
-extern unsigned long get_taint(void);
extern int root_mountflags;

extern bool early_boot_irqs_disabled;
@@ -493,22 +489,30 @@ extern enum system_states {
SYSTEM_RESTART,
} system_state;

-#define TAINT_PROPRIETARY_MODULE   0
-#define TAINT_FORCED_MODULE1
-#define TAINT_CPU_OUT_OF_SPEC  2
-#define TAINT_FORCED_RMMOD 3
-#define TAINT_MACHINE_CHECK4
-#define TAINT_BAD_PAGE 5
-#define TAINT_USER 6
-#define TAINT_DIE  7
-#define TAINT_OVERRIDDEN_ACPI_TABLE8
-#define TAINT_WARN 9
-#define TAINT_CRAP 10
-#define TAINT_FIRMWARE_WORKAROUND  11
-#define TAINT_OOT_MODULE   12
-#define TAINT_UNSIGNED_MODULE  13
-#define TAINT_SOFTLOCKUP   14
-#define TAINT_LIVEPATCH15
+enum taint_flags {
+   TAINT_PROPRIETARY_MODULE,   /*  0 */
+   TAINT_FORCED_MODULE,/*  1 */
+   TAINT_CPU_OUT_OF_SPEC,  /*  2 */
+   TAINT_FORCED_RMMOD, /*  3 */
+   TAINT_MACHINE_CHECK,/*  4 */
+   TAINT_BAD_PAGE, /*  5 */
+   TAINT_USER, /*  6 */
+   TAINT_DIE,  /*  7 */
+   TAINT_OVERRIDDEN_ACPI_TABLE,/*  8 */
+   TAINT_WARN, /*  9 */
+   TAINT_CRAP, /* 10 */
+   TAINT_FIRMWARE_WORKAROUND,  /* 11 */
+   TAINT_OOT_MODULE,   /* 12 */
+   TAINT_UNSIGNED_MODULE,  /* 13 */
+   TAINT_SOFTLOCKUP,   /* 14 */
+   TAINT_LIVEPATCH,/* 15 */
+   TAINT_FLAGS_COUNT   /* keep last! */
+};


I liked the enum idea because we got TAINT_FLAGS_COUNT for free :-)
however I think we need to switch back to the #defines because of the kbuild
error.

The "Error: invalid operands...for `<<'" messages are related to the
__WARN_TAINT() macro (arch/arm64/include/asm/bug.h) which emits some assembly
that relies on the taint values. We don't have access to the enum values
in the assembler so we start getting things like:

   .short ((1 << 0) | ((TAINT_WARN) << 8))

where TAINT_WARN should have already been preprocessed, and this is where that
invalid operand error is coming from.

Jessica

Re: taint/module: Clean up global and module taint flags handling

[Jessica Yu]

+++ Petr Mladek [12/09/16 16:13 +0200]:

The commit 66cc69e34e86a231 ("Fix: module signature vs tracepoints:
add new TAINT_UNSIGNED_MODULE") updated module_taint_flags() to
potentially print one more character. But it did not increase the
size of the corresponding buffers in m_show() and print_modules().

We have recently done the same mistake when adding a taint flag
for livepatching, see
https://lkml.kernel.org/g/cfba2c823bb984690b73572aaae1db596b54a082.1472137475.git.jpoim...@redhat.com

Also struct module uses an incompatible type for mod-taints flags.
It survived from the commit 2bc2d61a9638dab670d ("[PATCH] list module
taint flags in Oops/panic"). There was used "int" for the global taint
flags at these times. But only the global tain flags was later changed
to "unsigned long" by the commit 25ddbb18aae33ad2 ("Make the taint
flags reliable").

This patch defines TAINT_FLAGS_COUNT that can be used to create
arrays and buffers of the right size. Note that we could not use
enum because the taint flag indexes are used also in assembly code.

Then it reworks the table that describes the taint flags. The TAINT_*
numbers can be used as the index. Instead, we add information
if the taint flag is also shown per-module.

Finally, it uses "unsigned long", bit operations, and the updated
taint_flags table also for mod->taints.

It is not optimal because only few taint flags can be printed by
module_taint_flags(). But better be on the safe side. IMHO, it is
not worth the optimization and this is a good compromise.

Signed-off-by: Petr Mladek 
---

Changes against v1:

 + reverted the change to enums because it broke asm code

 + instead, forced the size of the taint_flags table;
   used taint numbers as the index; used the table also
   in module.c

 + fixed the type of mod->taints


include/linux/kernel.h |  9 +
include/linux/module.h |  2 +-
kernel/module.c| 31 +
kernel/panic.c | 53 --
4 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d96a6118d26a..33e88ff3af40 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -509,6 +509,15 @@ extern enum system_states {
#define TAINT_UNSIGNED_MODULE   13
#define TAINT_SOFTLOCKUP14
#define TAINT_LIVEPATCH 15
+#define TAINT_FLAGS_COUNT  16
+
+struct taint_flag {
+   char true;  /* character printed when tained */
+   char false; /* character printed when not tained */


s/tained/tainted


+   bool module;/* also show as a per-module taint flag */
+};
+
+extern const struct taint_flag taint_flags[TAINT_FLAGS_COUNT];

extern const char hex_asc[];
#define hex_asc_lo(x)   hex_asc[((x) & 0x0f)]
diff --git a/include/linux/module.h b/include/linux/module.h
index 0c3207d26ac0..f6ee569c62bb 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -399,7 +399,7 @@ struct module {
/* Arch-specific module values */
struct mod_arch_specific arch;

-   unsigned int taints;/* same bits as kernel:tainted */
+   unsigned long taints;   /* same bits as kernel:taint_flags */

#ifdef CONFIG_GENERIC_BUG
/* Support for BUG */
diff --git a/kernel/module.c b/kernel/module.c
index 529efae9f481..303e03e08b51 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -330,7 +330,7 @@ static inline void add_taint_module(struct module *mod, 
unsigned flag,
enum lockdep_ok lockdep_ok)
{
add_taint(flag, lockdep_ok);
-   mod->taints |= (1U << flag);
+   set_bit(flag, &mod->taints);
}

/*
@@ -1138,22 +1138,13 @@ static inline int module_unload_init(struct module *mod)
static size_t module_flags_taint(struct module *mod, char *buf)
{
size_t l = 0;
+   int i;
+
+   for (i = 0; i < TAINT_FLAGS_COUNT; i++) {
+   if (taint_flags[i].module && test_bit(i, &mod->taints))
+   buf[l++] = taint_flags[i].true;
+   }

-   if (mod->taints & (1 << TAINT_PROPRIETARY_MODULE))
-   buf[l++] = 'P';
-   if (mod->taints & (1 << TAINT_OOT_MODULE))
-   buf[l++] = 'O';
-   if (mod->taints & (1 << TAINT_FORCED_MODULE))
-   buf[l++] = 'F';
-   if (mod->taints & (1 << TAINT_CRAP))
-   buf[l++] = 'C';
-   if (mod->taints & (1 << TAINT_UNSIGNED_MODULE))
-   buf[l++] = 'E';
-   /*
-* TAINT_FORCED_RMMOD: could be added.
-* TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
-* apply to modules.
-*/
return l;
}

@@ -4036,6 +4027,10 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, 
const char *,
}
#endif /* CONFIG_KALLSYMS */

+/* Maximum number of characters written by module_flags() */
+#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
+
+/* Keep in sync with MODULE_FLAGS_BUF_SIZE !!! */
static 

livepatch/kprobes incompatibility

[Jessica Yu]

Hi Masami, Petr,

I'm trying to figure out where we are exactly with fixing the problems with
livepatch + kprobes, and I was wondering if there will be any more updates to
the ipmodify patchset that was originally merged back in 2014 (See:
https://lkml.org/lkml/2014/11/20/808). It seems that patch 4/5 ("kprobes: Set
IPMODIFY flag only if the probe can change regs->ip") wasn't merged due to
other ongoing work, and this patch in particular was needed to enforce a hard
conflict between livepatch and jprobes while still enabling livepatch and
kprobes to co-exist.

Currently, it looks like livepatch/kpatch and kprobes are still in direct
conflict, since both kprobe_ftrace_ops and klp_ops have FTRACE_OPS_FL_IPMODIFY
set. *But* it seems like this mutual exclusion wasn't 100% implemented; I'm
not sure if this was intentional, but kprobes registration will still return
success even when ftrace registration fails due to an ipmodify conflict, and
instead we just get WARNs (See: arm_kprobe_ftrace()).

So we still end up with buggy situations like the following:
  (1) livepatch patches meminfo_proc_show [ succeeds ]
  (2) systemtap probes meminfo_proc_show (using kprobes) [ fails ]
  * BUT from the user's perspective, it would look like systemtap succeeded,
since register_kprobe() returned success, but the handler will never 
fire
and only when we look at dmesg do we see that something went wrong
(i.e. ftrace registration had failed since livepatch already reserved
ipmodify in step 1).


From what I understand though, there was work being planned to limit this

direct conflict to just livepatch and jprobes, since most of the time kprobes
doesn't change regs->ip. Just wondering what the current state of this work is.

Thanks,
Jessica

Re: livepatch/module: make TAINT_LIVEPATCH module-specific

[Jessica Yu]

+++ Josh Poimboeuf [24/08/16 16:33 -0500]:

There's no reliable way to determine which module tainted the kernel
with CONFIG_LIVEPATCH.  For example, /sys/module//taint
doesn't report it.  Neither does the "mod -t" command in the crash tool.

Make it crystal clear who the guilty party is by converting
CONFIG_LIVEPATCH to a module taint flag.

This changes the behavior a bit: now the the flag gets set when the
module is loaded, rather than when it's enabled.


Did a quick sanity check and verified the module taint
shows up in crash and sysfs as expected, looks good.


Reviewed-by: Chunyu Hu 
Signed-off-by: Josh Poimboeuf 


Acked-by: Jessica Yu 


---
kernel/livepatch/core.c |  3 ---
kernel/module.c | 35 ---
2 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5fbabe0..af46438 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -545,9 +545,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
list_prev_entry(patch, list)->state == KLP_DISABLED)
return -EBUSY;

-   pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
-   add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
-
pr_notice("enabling patch '%s'\n", patch->mod->name);

klp_for_each_object(patch, obj) {
diff --git a/kernel/module.c b/kernel/module.c
index 529efae..fd5f95b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1149,6 +1149,8 @@ static size_t module_flags_taint(struct module *mod, char 
*buf)
buf[l++] = 'C';
if (mod->taints & (1 << TAINT_UNSIGNED_MODULE))
buf[l++] = 'E';
+   if (mod->taints & (1 << TAINT_LIVEPATCH))
+   buf[l++] = 'K';
/*
 * TAINT_FORCED_RMMOD: could be added.
 * TAINT_CPU_OUT_OF_SPEC, TAINT_MACHINE_CHECK, TAINT_BAD_PAGE don't
@@ -2791,26 +2793,6 @@ static int copy_chunked_from_user(void *dst, const void 
__user *usrc, unsigned l
return 0;
}

-#ifdef CONFIG_LIVEPATCH
-static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
-{
-   mod->klp = get_modinfo(info, "livepatch") ? true : false;
-
-   return 0;
-}
-#else /* !CONFIG_LIVEPATCH */
-static int find_livepatch_modinfo(struct module *mod, struct load_info *info)
-{
-   if (get_modinfo(info, "livepatch")) {
-   pr_err("%s: module is marked as livepatch module, but livepatch 
support is disabled",
-  mod->name);
-   return -ENOEXEC;
-   }
-
-   return 0;
-}
-#endif /* CONFIG_LIVEPATCH */
-
/* Sets info->hdr and info->len. */
static int copy_module_from_user(const void __user *umod, unsigned long len,
  struct load_info *info)
@@ -2969,9 +2951,16 @@ static int check_modinfo(struct module *mod, struct 
load_info *info, int flags)
"is unknown, you have been warned.\n", mod->name);
}

-   err = find_livepatch_modinfo(mod, info);
-   if (err)
-   return err;
+   if (get_modinfo(info, "livepatch")) {
+   if (!IS_ENABLED(CONFIG_LIVEPATCH)) {
+   pr_err("%s: module is marked as livepatch module, but 
livepatch support is disabled\n",
+  mod->name);
+   return -ENOEXEC;
+   }
+   mod->klp = true;
+   pr_warn("%s: loading livepatch module.\n", mod->name);
+   add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
+   }

/* Set up license info based on the info section */
set_license(mod, get_modinfo(info, "license"));
--
2.7.4

Re: [PATCH] modpost: abort if a module name is too long

[Jessica Yu]

+++ Jessica Yu [06/06/17 20:41 -0700]:

+++ Wanlong Gao [06/06/17 09:07 +0800]:



On 2017/6/5 10:09, Jessica Yu wrote:

+++ Wanlong Gao [02/06/17 11:04 +0800]:



On 2017/6/2 7:23, Jessica Yu wrote:

+++ Wanlong Gao [31/05/17 11:48 +0800]:



On 2017/5/31 11:30, Jessica Yu wrote:

+++ Wanlong Gao [31/05/17 10:23 +0800]:

Hi Jessica,

On 2017/5/29 17:10, Jessica Yu wrote:

+++ Xie XiuQi [20/05/17 15:46 +0800]:

From: Wanlong Gao 

Module name has a limited length, but currently the build system
allows the build finishing even if the module name is too long.

CC  
/root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
/root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:9:2:
warning: initializer-string for array of chars is too long [enabled by default]
.name = KBUILD_MODNAME,
^

but it's merely a warning.

This patch adds the check of the module name length in modpost and stops
the build properly.

Signed-off-by: Wanlong Gao 
Signed-off-by: Xie XiuQi 
---
scripts/mod/modpost.c | 11 +++
1 file changed, 11 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 30d752a..db11c57 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2166,6 +2166,17 @@ static int add_versions(struct buffer *b, struct module 
*mod)
{
   struct symbol *s, *exp;
   int err = 0;
+const char *mod_name;
+
+mod_name = strrchr(mod->name, '/');
+if (mod_name == NULL)
+mod_name = mod->name;
+else
+mod_name++;
+if (strlen(mod_name) >= MODULE_NAME_LEN) {
+merror("module name is too long [%s.ko]\n", mod->name);
+return 1;
+}


Hi Xie,

This check shouldn't be in add_versions() (which does something else entirely),
it should probably be put in a separate helper function called from main. But
I'm not a big fan of the extra string manipulation to do something this simple.

I think this check can be vastly simplified, how about something like the
following?


This looks better, would you apply your following patch?

Reviewed-by: Wanlong Gao 
Tested-by: Wanlong Gao 


Sure, thanks for testing. I'll go ahead and format this into a proper
patch and resend.


Please wait, I just found that this patch makes the built module can't
be inserted by the following error:

# insmod abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc.ko
insmod: ERROR: could not insert module 
abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc.ko: Invalid parameters

# dmesg
abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabc: Unknown symbol 
__fentry__ (err -22)


Hm, I am unable to reproduce this. It looks like __fentry__ is missing
from your kernel, you may have a mismatch between the kernel config
that you're running and the config you are using to build the module.
In other words, it seems like you might have built the module with
CONFIG_FTRACE but built the kernel without.

Few questions -

What is the output of running `grep __fentry__ /proc/kallsyms`?



Sure it has.


Does your module correspond to the running kernel version?


Sure.



Do you have CONFIG_FTRACE/FUNCTION_TRACER enabled in your running
kernel?



Sure.



Is that the full dmesg output (are there any other error messages)?


Even when I compiled the kernel with your patch, the kernel module load
failed at the boot time with the following error:

[1.656708] libcrc32c: no symbol version for __fentry__
[1.656709] libcrc32c: Unknown symbol __fentry__ (err -22)

But my above patch in add_versions() doesn't have such problem, I've no
idea why. Maybe your patch breaks some sections?


Hm, I am still unable to reproduce this on my system with modversions
enabled and the -rc2 kernel. But judging by the errno (-22) it looks
like this is failing in check_version()/resolve_symbol() for you,
which leads me to think that this is somehow messing with the
__versions table generated by modpost (not sure why).

Does the  versions[] array in the generated *.mod.c file for your
test module look different with and without the patch? Also: what
version of gcc and binutils are you using, and what kernel version are
you testing on?


The *.mod.c file are same except the added __modname_test section, the gcc
,binutils and kernel are all from centos 7.2 (3.10.0-327).


Thanks for the additional info. Just FYI, I'm going to be out this
week and part of next week due to travelling, but I'll be able to take
another look at this next Thurs/Fri. If we can't resolve the issue, we
can just work on your original patch.


Thanks for your patience, I've just moved abroad and getting to stable
internet has been a challenge :-/

Here's my last attempt at fixing the BUILD_BUG_ON patch (I am not sure
why it seems to be messing with the __versions table on your setup,
perhaps it is related to .discard usage?).

D

Re: [PATCH] livepatch: Make livepatch dependent on !TRIM_UNUSED_KSYMS

[Jessica Yu]

+++ Miroslav Benes [26/05/17 14:45 +0200]:

If TRIM_UNUSED_KSYMS is enabled, all unneeded exported symbols are made
unexported. Two-pass build of the kernel is done to find out which
symbols are needed based on a configuration. This effectively
complicates things for out-of-tree modules.

Livepatch exports functions to (un)register and enable/disable a live
patch. The only in-tree module which uses these functions is a sample in
samples/livepatch/. If the sample is disabled, the functions are
trimmed and out-of-tree live patches cannot be built.

Note that live patches are intended to be built out-of-tree.

Suggested-by: Michal Marek 
Signed-off-by: Miroslav Benes 


Makes sense to me:

Acked-by: Jessica Yu 


---
kernel/livepatch/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
index 045022557936..ec4565122e65 100644
--- a/kernel/livepatch/Kconfig
+++ b/kernel/livepatch/Kconfig
@@ -10,6 +10,7 @@ config LIVEPATCH
depends on SYSFS
depends on KALLSYMS_ALL
depends on HAVE_LIVEPATCH
+   depends on !TRIM_UNUSED_KSYMS
help
  Say Y here if you want to support kernel live patching.
  This option has no runtime impact until a kernel "patch"
--
2.12.2

Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[Jessica Yu]

+++ Masami Hiramatsu [26/05/17 09:24 +0900]:

On Thu, 25 May 2017 19:24:26 +0200
"Luis R. Rodriguez"  wrote:


On Thu, May 25, 2017 at 07:38:17PM +0900, Masami Hiramatsu wrote:
> Fix kprobes to set(recover) RWX bits correctly on trampoline
> buffer before releasing it. Releasing readonly page to
> module_memfree() crash the kernel.
>
> Without this fix, if kprobes user register a bunch of kprobes
> in function body (since kprobes on function entry usually
> use ftrace) and unregister it, kernel hits a BUG and crash.
>
> Signed-off-by: Masami Hiramatsu 
> Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
> ---
>  arch/x86/kernel/kprobes/core.c |9 +
>  kernel/kprobes.c   |2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 5b2bbfb..6b87780 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -52,6 +52,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn 
*insn)
>}
>  }
>
> +/* Recover page to RW mode before releasing it */
> +void free_insn_page(void *page)
> +{
> +  set_memory_nx((unsigned long)page & PAGE_MASK, 1);
> +  set_memory_rw((unsigned long)page & PAGE_MASK, 1);
> +  module_memfree(page);
> +}

Is this needed for all module_memfree() ? If so should / could it just do it
for alloc users ?


Hmm, would you mean setting those bits in module_memfree()?
I think it should be discussed with other users, kmodule, bpf and ftrace.
It could be, but I'm not so sure about that because setting nx
timing would be critical for some users. As far as I can see,
for ftrace and kprobes, that is OK.


Memory does need to be rw before calling module_memfree(), although I
think it might be better leave that responsibility/flexibility to the
callers, instead of blanket calls to set_memory_rw/x. At least in the
case of the module loader, we have finer-grained control of page
protections; not all pages within the module_alloc'd region need
set_memory_rw/x to be called before freeing (see disable_ro_nx() in
module.c).

Jessica   

Re: [PATCH] fs: warn in case userspace lied about modprobe return

[Jessica Yu]

+++ Luis R. Rodriguez [25/05/17 17:44 -0700]:

kmod <= v19 was broken -- it could return 0 to modprobe calls,
incorrectly assuming that a kernel module was built-in, whereas in
reality the module was just forming in the kernel. The reason for this
is an incorrect userspace heuristics. A userspace kmod fix is available
for it [0], however should userspace break again we could go on with
an failed get_fs_type() which is hard to debug as the request_module()
is detected as returning 0. The first suspect would be that there is
something worth with the kernel's module loader and obviously in this
case that is not the issue.

Since these issues are painful to debug complain when we know userspace
has  outright lied to us.

[0] 
http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

Suggested-by: Rusty Russell 
Cc: Jessica Yu 
Signed-off-by: Luis R. Rodriguez 
---
fs/filesystems.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index cac75547d35c..0f477a5de6ea 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,8 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
int len = dot ? dot - name : strlen(name);

fs = __get_fs_type(name, len);
-   if (!fs && (request_module("fs-%.*s", len, name) == 0))
+   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
+   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no 
fs?\n", len, name);


The WARN needs to go below the second __get_fs_type() attempt, no?
Because we want to try __get_fs_type() again right after
request_module(), to see if the fs loaded, and _then_ WARN if it
doesn't appear to be loaded.

Jessica


fs = __get_fs_type(name, len);
+   }

if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
--
2.11.0

Re: [PATCH] modpost: abort if a module name is too long

[Jessica Yu]

+++ Xie XiuQi [20/05/17 15:46 +0800]:

From: Wanlong Gao 

Module name has a limited length, but currently the build system
allows the build finishing even if the module name is too long.

 CC  
/root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.o
/root/kprobe_example/abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz.mod.c:9:2:
warning: initializer-string for array of chars is too long [enabled by default]
 .name = KBUILD_MODNAME,
 ^

but it's merely a warning.

This patch adds the check of the module name length in modpost and stops
the build properly.

Signed-off-by: Wanlong Gao 
Signed-off-by: Xie XiuQi 
---
scripts/mod/modpost.c | 11 +++
1 file changed, 11 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 30d752a..db11c57 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2166,6 +2166,17 @@ static int add_versions(struct buffer *b, struct module 
*mod)
{
struct symbol *s, *exp;
int err = 0;
+   const char *mod_name;
+
+   mod_name = strrchr(mod->name, '/');
+   if (mod_name == NULL)
+   mod_name = mod->name;
+   else
+   mod_name++;
+   if (strlen(mod_name) >= MODULE_NAME_LEN) {
+   merror("module name is too long [%s.ko]\n", mod->name);
+   return 1;
+   }


Hi Xie,

This check shouldn't be in add_versions() (which does something else entirely),
it should probably be put in a separate helper function called from main. But
I'm not a big fan of the extra string manipulation to do something this simple.

I think this check can be vastly simplified, how about something like the
following?

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 48397fe..bb09fc7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2139,6 +2139,9 @@ static void add_header(struct buffer *b, struct module 
*mod)
  "#endif\n");
buf_printf(b, "\t.arch = MODULE_ARCH_INIT,\n");
buf_printf(b, "};\n");
+   buf_printf(b, "\n");
+   buf_printf(b, "static void __attribute__((section(\".discard\"), used)) 
__modname_test(void)\n");
+   buf_printf(b, "{ BUILD_BUG_ON(sizeof(KBUILD_MODNAME) > MODULE_NAME_LEN); 
}\n");
}

static void add_intree_flag(struct buffer *b, int is_intree)

This simply checks if KBUILD_MODNAME > MODULE_NAME_LEN and breaks the build if
it does.

Jessica


for (s = mod->unres; s; s = s->next) {
exp = find_symbol(s->name);
--
1.8.3.1

Re: module: Do not paper over type mismatches in module_param_call()

[Jessica Yu]

+++ Kees Cook [30/10/17 14:20 -0700]:

On Tue, Oct 17, 2017 at 7:04 PM, Kees Cook  wrote:

(re-sending to Jessica's @korg address...)

The module_param_call() macro was explicitly casting the .set and .get
function prototypes away with (void *). This can lead to hard-to-find
type mismatches. Additionally, it creates problems for static checkers
and Control Flow Itegrity compiler features, which depend on clustering
function call sites based on prototype signature.

This removes the casts and fixes all the incorrect prototypes tree-wide.


A quick ping on this. I'd really like to land this in 4.15, as it's
relatively trivial. How does this look to you Jessica?


Hey Kees!

Sorry for the delay, and thanks for fixing this up - had to dig deep
through git history to remind myself why these casts and that weird
compiler type-check were being done in the first place :-/ I guess
the original reason was backward compatibility.

Anyways I've pushed this to modules-next, thanks!

Jessica

Re: linux-next: manual merge of the ipmi tree with the modules tree

[Jessica Yu]

+++ Corey Minyard [02/11/17 08:31 -0500]:

On 11/01/2017 10:58 PM, Stephen Rothwell wrote:

Hi Corey,

Today's linux-next merge of the ipmi tree got a conflict in:

  drivers/char/ipmi/ipmi_si_intf.c

between commit:

  e4dca7b7aa08 ("treewide: Fix function prototypes for module_param_call()")

from the modules tree and commit:

  44814ec982d2 ("ipmi_si: Move the hotmod handling to another file.")

from the ipmi tree.


Thanks Stephen.

Kees, do you have a tree I can merge so we can avoid this going upstream?


Hi Corey,

the modules-next tree (with the conflicting commit) is found here: 


   git://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git#modules-next

Thanks,

Jessica 

[PATCH v2 0/2] kprobes: improve error handling when arming/disarming kprobes

[Jessica Yu]

Hi,

This patchset attempts to improve error handling when arming or disarming
ftrace-based kprobes. The current behavior is to simply WARN when ftrace
(un-)registration fails, without propagating the error code. This can lead
to confusing situations where, for example, register_kprobe()/enable_kprobe()
would return 0 indicating success even if arming via ftrace had failed. In
this scenario we'd end up with a non-functioning kprobe even though kprobe
registration (or enablement) returned success. In this patchset, we take
errors from ftrace into account and propagate the error when we cannot arm
or disarm a kprobe.

Below is an example that illustrates the problem using livepatch and
systemtap (which uses kprobes underneath). Both livepatch and kprobes use
ftrace ops with the IPMODIFY flag set, so registration at the same
function entry is limited to only one ftrace user. 

Before
--
# modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has 
IPMODIFY set
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'

   .. (nothing prints after reading /proc/cmdline) ..

The systemtap handler doesn't execute due to a kprobe arming failure caused
by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
indication of error from systemtap (because register_kprobe() returned
success) unless the user inspects dmesg.

After
-
# modprobe livepatch-sample 
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'
WARNING: probe 
kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call
 (address 0xa82fe910) registration error (rc -16)

Although the systemtap handler doesn't execute (as it shouldn't), the
ftrace error is propagated and now systemtap prints a visible error message
stating that (kprobe) registration had failed (because register_kprobe()
returned an error), along with the propagated error code.

This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

This patchset has been lightly sanity-tested (on linux-next) with kprobes,
kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
more testing is greatly appreciated.


Changes from v1:
- Don't arm the kprobe before adding it to the kprobe table, otherwise
  we'll temporarily see a stray breakpoint.
- Remove kprobe from the kprobe_table and call synchronize_sched() if
  arming during register_kprobe() fails.
- add Masami's ack on the 2nd patch (unchanged from v1)

---
Jessica Yu (2):
  kprobes: propagate error from arm_kprobe_ftrace()
  kprobes: propagate error from disarm_kprobe_ftrace()

 kernel/kprobes.c | 164 ++-
 1 file changed, 114 insertions(+), 50 deletions(-)

-- 
2.13.6

[PATCH v2 2/2] kprobes: propagate error from disarm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.

disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all kprobes could be disarmed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Acked-by: Masami Hiramatsu 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 76 +---
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f4a094007cb5..2d50d3af78d7 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1002,23 +1002,27 @@ static int arm_kprobe_ftrace(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
-   kprobe_ftrace_enabled--;
-   if (kprobe_ftrace_enabled == 0) {
+   if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", 
ret))
+   return ret;
}
+
+   kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, 
ret);
+   return ret;
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)   (0)
-#define disarm_kprobe_ftrace(p)do {} while (0)
+#define disarm_kprobe_ftrace(p)(0)
 #endif
 
 /* Arm a kprobe with text_mutex */
@@ -1037,18 +1041,18 @@ static int arm_kprobe(struct kprobe *kp)
 }
 
 /* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   disarm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return disarm_kprobe_ftrace(kp);
 
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /*
@@ -1629,11 +1633,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 static struct kprobe *__disable_kprobe(struct kprobe *p)
 {
struct kprobe *orig_p;
+   int ret;
 
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
-   return NULL;
+   return ERR_PTR(-EINVAL);
 
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1647,8 +1652,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
 * should have already been disarmed, so
 * skip unneed disarming process.
 */
-   if (!kprobes_all_disarmed)
-   disarm_kprobe(orig_p, true);
+   if (!kprobes_all_disarmed) {
+   ret = disarm_kprobe(orig_p, true);
+   if (ret) {
+   p->flags &= ~KPROBE_FLAG_DISABLED;
+   return ERR_PTR(ret);
+   }
+   }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1665,8 +1675,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
 
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
-   if (ap == NULL)
-   return -EINVAL;
+   if (IS_ERR(ap))
+   return PTR_ERR(ap);
 
if (ap == p)
/*
@@ -2099,12 +2109,14 @@ static void kill_kprobe(struct kprobe *p)
 int disable_kprobe(struct kprobe *kp)
 {
int ret = 0;
+   struct kprobe *p;
 
mutex_lock(&kprobe_mutex);
 
/* Disable this kprobe */
-   if (__disable_kprobe(kp) == NULL)
-   ret = -EINVAL;
+   p = __disable_kprobe(kp);
+   if (I

[PATCH v2 1/2] kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.

arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all kprobes could be armed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 88 
 1 file changed, 63 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index da2ccf142358..f4a094007cb5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -978,18 +978,27 @@ static int prepare_kprobe(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 0, 0);
-   WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
-   kprobe_ftrace_enabled++;
-   if (kprobe_ftrace_enabled == 1) {
+   if (WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, 
ret))
+   return ret;
+
+   if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret))
+   goto err_ftrace;
}
+
+   kprobe_ftrace_enabled++;
+   return ret;
+
+err_ftrace:
+   ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+   return ret;
 }
 
 /* Caller must lock kprobe_mutex */
@@ -1008,22 +1017,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p)   do {} while (0)
+#define arm_kprobe_ftrace(p)   (0)
 #define disarm_kprobe_ftrace(p)do {} while (0)
 #endif
 
 /* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   arm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /* Disarm a kprobe with text_mutex */
@@ -1362,9 +1372,14 @@ static int register_aggr_kprobe(struct kprobe *orig_p, 
struct kprobe *p)
 
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
-   if (!kprobes_all_disarmed)
+   if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
-   arm_kprobe(ap);
+   ret = arm_kprobe(ap);
+   if (ret) {
+   ap->flags |= KPROBE_FLAG_DISABLED;
+   list_del_rcu(&p->list);
+   }
+   }
}
return ret;
 }
@@ -1573,8 +1588,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
   &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-   if (!kprobes_all_disarmed && !kprobe_disabled(p))
-   arm_kprobe(p);
+   if (!kprobes_all_disarmed && !kprobe_disabled(p)) {
+   ret = arm_kprobe(p);
+   if (ret) {
+   hlist_del_rcu(&p->hlist);
+   synchronize_sched();
+   goto out;

Re: linux-next: manual merge of the ipmi tree with the modules tree

[Jessica Yu]

+++ Corey Minyard [02/11/17 11:40 -0500]:

On 11/02/2017 09:27 AM, Jessica Yu wrote:

+++ Corey Minyard [02/11/17 08:31 -0500]:

On 11/01/2017 10:58 PM, Stephen Rothwell wrote:

Hi Corey,

Today's linux-next merge of the ipmi tree got a conflict in:

  drivers/char/ipmi/ipmi_si_intf.c

between commit:

  e4dca7b7aa08 ("treewide: Fix function prototypes for 
module_param_call()")


from the modules tree and commit:

  44814ec982d2 ("ipmi_si: Move the hotmod handling to another file.")

from the ipmi tree.


Thanks Stephen.

Kees, do you have a tree I can merge so we can avoid this going 
upstream?


Hi Corey,

the modules-next tree (with the conflicting commit) is found here:
git://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git#modules-next



Hi Jessica,

I merged this into the ipmi for-next tree, so we should be good now,
assuming I did this right.


Thanks Corey!

Jessica

Re: x86/module: Detect corrupt relocations against nonzero data

[Jessica Yu]

+++ Josh Poimboeuf [02/11/17 21:19 -0500]:

On Thu, Nov 02, 2017 at 04:57:11PM -0500, Josh Poimboeuf wrote:

There have been some cases where external tooling (e.g., kpatch-build)
creates a corrupt relocation which targets the wrong address.  This is a
silent failure which can corrupt memory in unexpected places.

On x86, the bytes of data being overwritten by relocations are always
initialized to zero beforehand.  Use that knowledge to add sanity checks
to detect such cases before they corrupt memory.

Signed-off-by: Josh Poimboeuf 
---
 arch/x86/kernel/module.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 62e7d70aadd5..a69b12617820 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -172,19 +172,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
case R_X86_64_NONE:
break;
case R_X86_64_64:
+   if (*(u64 *)loc != 0)
+   goto nonzero;
*(u64 *)loc = val;
break;
case R_X86_64_32:
+   if (*(u32 *)loc != 0)
+   goto nonzero;
*(u32 *)loc = val;
if (val != *(u32 *)loc)
goto overflow;
break;
case R_X86_64_32S:
+   if (*(s32 *)loc != 0)
+   goto nonzero;
*(s32 *)loc = val;
if ((s64)val != *(s32 *)loc)
goto overflow;
break;
case R_X86_64_PC32:
+   if (*(u64 *)loc != 0)
+   goto nonzero;
val -= (u64)loc;
*(u32 *)loc = val;


NACK - this last bit is obviously a bug, not sure how it passed my
testing without module load failures...


Thanks for the patch Josh, btw - could you also CC the x86 folks when
you send out v2? 


Thanks!

Jessica

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ SF Markus Elfring [06/10/17 17:12 +0200]:

From: Markus Elfring 
Date: Fri, 6 Oct 2017 16:27:26 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
kernel/module.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..07ef44767245 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct 
module *b)

pr_debug("Allocating new usage for %s.\n", a->name);
use = kmalloc(sizeof(*use), GFP_ATOMIC);
-   if (!use) {
-   pr_warn("%s: out of memory loading\n", a->name);
+   if (!use)
return -ENOMEM;
-   }


IMO this is removing useful information. Although stack traces are
generated on alloc failures, the extra print also tells us which
module we were trying to load at the time the memory allocation
failed.

Jessica

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ Dan Carpenter [19/10/17 13:30 +0300]:

On Thu, Oct 19, 2017 at 11:29:43AM +0200, Jessica Yu wrote:

+++ SF Markus Elfring [06/10/17 17:12 +0200]:
> From: Markus Elfring 
> Date: Fri, 6 Oct 2017 16:27:26 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 
> ---
> kernel/module.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..07ef44767245 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct 
module *b)
>
>pr_debug("Allocating new usage for %s.\n", a->name);
>use = kmalloc(sizeof(*use), GFP_ATOMIC);
> -  if (!use) {
> -  pr_warn("%s: out of memory loading\n", a->name);
> +  if (!use)
>return -ENOMEM;
> -  }

IMO this is removing useful information. Although stack traces are
generated on alloc failures, the extra print also tells us which
module we were trying to load at the time the memory allocation
failed.


This is a small allocation so it can't fail in current kernels.  I can't
imagine a situation where this could fail and it wasn't dead easy to
debug.  Most modules are loaded at boot so it's not likely to fail, but
if it did, it would be easy to reproduce.  If it's not loaded at boot
it's probably really easy to tell which module we're loading.


Yeah, good points. And on second thought, we normally don't print
warnings for every small alloc failure in the kernel anyway (that
would be utterly superfluous), the error code itself is sufficient.
And in the module loader this seems to be the only printk out of the
dozen alloc calls we do, so I'm OK with removing this one.

Thanks,

Jessica

Re: kernel/module: Delete an error message for a failed memory allocation in add_module_usage()

[Jessica Yu]

+++ SF Markus Elfring [06/10/17 17:12 +0200]:

From: Markus Elfring 
Date: Fri, 6 Oct 2017 16:27:26 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 


Applied to modules-next, thanks.

Jessica


---
kernel/module.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..07ef44767245 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -837,10 +837,8 @@ static int add_module_usage(struct module *a, struct 
module *b)

pr_debug("Allocating new usage for %s.\n", a->name);
use = kmalloc(sizeof(*use), GFP_ATOMIC);
-   if (!use) {
-   pr_warn("%s: out of memory loading\n", a->name);
+   if (!use)
return -ENOMEM;
-   }

use->source = a;
use->target = b;
--
2.14.2

Re: [PATCH] kernel/module: Use kmemdup to replace kmalloc+memcpy

[Jessica Yu]

+++ zhong jiang [01/08/18 00:56 +0800]:

we prefer to the kmemdup rather than kmalloc+memcpy. so just
replace them.

Signed-off-by: zhong jiang 


Applied, thanks.

Jessica


---
kernel/module.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 20344e4..6746c85 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2057,21 +2057,19 @@ static int copy_module_elf(struct module *mod, struct 
load_info *info)

/* Elf section header table */
size = sizeof(*info->sechdrs) * info->hdr->e_shnum;
-   mod->klp_info->sechdrs = kmalloc(size, GFP_KERNEL);
+   mod->klp_info->sechdrs = kmemdup(info->sechdrs, size, GFP_KERNEL);
if (mod->klp_info->sechdrs == NULL) {
ret = -ENOMEM;
goto free_info;
}
-   memcpy(mod->klp_info->sechdrs, info->sechdrs, size);

/* Elf section name string table */
size = info->sechdrs[info->hdr->e_shstrndx].sh_size;
-   mod->klp_info->secstrings = kmalloc(size, GFP_KERNEL);
+   mod->klp_info->secstrings = kmemdup(info->secstrings, size, GFP_KERNEL);
if (mod->klp_info->secstrings == NULL) {
ret = -ENOMEM;
goto free_sechdrs;
}
-   memcpy(mod->klp_info->secstrings, info->secstrings, size);

/* Elf symbol section index */
symndx = info->index.sym;
--
1.7.12.4

Re: [PATCH 3/3] modsign: print module name along with error message

[Jessica Yu]

+++ Jessica Yu [30/05/18 11:08 +0200]:

It is useful to know which module failed signature verification, so
print the module name along with the error message.

Signed-off-by: Jessica Yu 
---
kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ae824a6f1a03..8670585eb5f7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2807,7 +2807,7 @@ static int module_sig_check(struct load_info *info, int 
flags,
reason = "Loading of module with unavailable key";
decide:
if (sig_enforce) {
-   pr_notice("%s is rejected\n", reason);
+   pr_notice("%s: %s is rejected\n", info->name, reason);
return -EKEYREJECTED;
}



Hi David!

I've just merged patches 1 and 2 into modules-next. Would you be
interested this patch (patch 3) if/when you push lockdown upstream
again?

Thanks!

Jessica

Re: [PATCH] module: print sensible error code

[Jessica Yu]

+++ Jason A. Donenfeld [22/06/18 17:38 +0200]:

Printing "err 0" to the user in the warning message is not particularly
useful, especially when this gets transformed into a -ENOENT for the
remainder of the call chain.

Signed-off-by: Jason A. Donenfeld 


Applied, thanks.

Jessica


---
kernel/module.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..c666ab4139f4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2282,9 +2282,9 @@ static int simplify_symbols(struct module *mod, const 
struct load_info *info)
if (!ksym && ELF_ST_BIND(sym[i].st_info) == STB_WEAK)
break;

-   pr_warn("%s: Unknown symbol %s (err %li)\n",
-   mod->name, name, PTR_ERR(ksym));
ret = PTR_ERR(ksym) ?: -ENOENT;
+   pr_warn("%s: Unknown symbol %s (err %d)\n",
+   mod->name, name, ret);
break;

default:
--
2.17.1

Re: [PATCH] module: replace VMLINUX_SYMBOL_STR() with __stringify() or string literal

[Jessica Yu]

+++ Masahiro Yamada [24/06/18 00:37 +0900]:

With the special case handling for Blackfin and Metag was removed by
commit 94e58e0ac312 ("export.h: remove code for prefixing symbols with
underscore"), VMLINUX_SYMBOL_STR() is now equivalent to __stringify().

Replace the remaining usages to prepare for the entire removal of
VMLINUX_SYMBOL_STR().

Signed-off-by: Masahiro Yamada 


Applied, thanks.

Jessica


---

include/linux/module.h | 4 ++--
kernel/module.c| 6 ++
2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index d44df9b..f807f15 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -266,7 +266,7 @@ extern int modules_disabled; /* for sysctl */
/* Get/put a kernel symbol (calls must be symmetric) */
void *__symbol_get(const char *symbol);
void *__symbol_get_gpl(const char *symbol);
-#define symbol_get(x) ((typeof(&x))(__symbol_get(VMLINUX_SYMBOL_STR(x
+#define symbol_get(x) ((typeof(&x))(__symbol_get(__stringify(x

/* modules using other modules: kdb wants to see this. */
struct module_use {
@@ -575,7 +575,7 @@ extern void __noreturn __module_put_and_exit(struct module 
*mod,
#ifdef CONFIG_MODULE_UNLOAD
int module_refcount(struct module *mod);
void __symbol_put(const char *symbol);
-#define symbol_put(x) __symbol_put(VMLINUX_SYMBOL_STR(x))
+#define symbol_put(x) __symbol_put(__stringify(x))
void symbol_put_addr(void *addr);

/* Sometimes we know we already have a refcount, and it's easier not
diff --git a/kernel/module.c b/kernel/module.c
index f475f30..624d2c0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1339,14 +1339,12 @@ static inline int check_modstruct_version(const struct 
load_info *info,
 * locking is necessary -- use preempt_disable() to placate lockdep.
 */
preempt_disable();
-   if (!find_symbol(VMLINUX_SYMBOL_STR(module_layout), NULL,
-&crc, true, false)) {
+   if (!find_symbol("module_layout", NULL, &crc, true, false)) {
preempt_enable();
BUG();
}
preempt_enable();
-   return check_version(info, VMLINUX_SYMBOL_STR(module_layout),
-mod, crc);
+   return check_version(info, "module_layout", mod, crc);
}

/* First part is kernel version, which we ignore if module has crcs. */
--
2.7.4

Re: [PATCH 2/6] module: add support for symbol namespaces.

[Jessica Yu]

+++ Martijn Coenen [24/07/18 09:56 +0200]:

I did find an issue with my approach:

On Mon, Jul 16, 2018 at 2:21 PM, Martijn Coenen  wrote:

The ELF symbols are renamed to include the namespace with an asm label;
for example, symbol 'usb_stor_suspend' in namespace USB_STORAGE becomes
'usb_stor_suspend.USB_STORAGE'.  This allows modpost to do namespace
checking, without having to go through all the effort of parsing ELF and
reloction records just to get to the struct kernel_symbols.


depmod doesn't like this: if namespaced symbols are built-in to the
kernel, they will appear as 'symbol.NS' in the symbol table, whereas
modules using those symbols just depend on 'symbol'. This will cause
depmod to warn about unknown symbols. I didn't find this previously
because all the exports/imports I tested were done from modules
themselves.

One way to deal with it is to change depmod, but it looks like it
hasn't been changed in ages, and it would also introduce a dependency
on userspaces to update it to avoid these warnings. So, we'd have to
encode the symbol namespace information in another way for modpost to
use. I could just write a separate post-processing tool (much like
genksyms), or perhaps encode the information in a discardable section.
Any other suggestions welcome.


This seems to be because depmod uses System.map as a source for kernel
symbols and scans for only the ones prefixed with "__ksymtab" to find
the exported ones - and those happen to use the '.' to mark the
namespace it belongs to. It strips that prefix and uses the remainder
as the actual symbol name. To fix that it'd have to strip the
namespace suffix in the symbol name as well.

Just scanning the depmod source code, it seems really trivial to just
have depmod accommodate the new symbol name format. Adding Lucas (kmod
maintainer) to CC.

Thanks,

Jessica





On x86_64 I saw no difference in binary size (compression), but at
runtime this will require a word of memory per export to hold the
namespace. An alternative could be to store namespaced symbols in their
own section and use a separate 'struct namespaced_kernel_symbol' for
that section, at the cost of making the module loader more complex.

Signed-off-by: Martijn Coenen 
---
 include/asm-generic/export.h |  2 +-
 include/linux/export.h   | 83 +++-
 include/linux/module.h   | 13 ++
 kernel/module.c  | 79 ++
 4 files changed, 156 insertions(+), 21 deletions(-)

diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 68efb950a918..4c3d1afb702f 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -29,7 +29,7 @@
.section ___ksymtab\sec+\name,"a"
.balign KSYM_ALIGN
 __ksymtab_\name:
-   __put \val, __kstrtab_\name
+   __put \val, __kstrtab_\name, 0
.previous
.section __ksymtab_strings,"a"
 __kstrtab_\name:
diff --git a/include/linux/export.h b/include/linux/export.h
index ad6b8e697b27..9f6e70eeb85f 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -22,6 +22,11 @@ struct kernel_symbol
 {
unsigned long value;
const char *name;
+   const char *namespace;
+};
+
+struct namespace_import {
+   const char *namespace;
 };

 #ifdef MODULE
@@ -54,18 +59,28 @@ extern struct module __this_module;
 #define __CRC_SYMBOL(sym, sec)
 #endif

+#define NS_SEPARATOR "."
+
+#define MODULE_IMPORT_NS(ns)   \
+   static const struct namespace_import __knsimport_##ns   \
+   asm("__knsimport_" #ns) \
+   __attribute__((section("__knsimport"), used))   \
+   = { #ns }
+
 /* For every exported symbol, place a struct in the __ksymtab section */
-#define ___EXPORT_SYMBOL(sym, sec) \
+#define ___EXPORT_SYMBOL(sym, sec, ns, nspost, nspost2)
\
extern typeof(sym) sym; \
__CRC_SYMBOL(sym, sec)  \
-   static const char __kstrtab_##sym[] \
+   static const char __kstrtab_##sym##nspost[] \
__attribute__((section("__ksymtab_strings"), aligned(1)))   \
= #sym; \
-   static const struct kernel_symbol __ksymtab_##sym   \
+   static const struct kernel_symbol __ksymtab_##sym##nspost   \
+   asm("__ksymtab_" #sym nspost2)  \
__used  \
-   __attribute__((section("___ksymtab" sec "+" #sym), used))   \
+   __attribute__((section("___ksymtab" sec "+" #sym #nspost))) \
+   __attribute__((used))   \
__attribute__((aligned(sizeof(void * 

Re: [PATCH] Revert "module: Add retpoline tag to VERMAGIC"

[Jessica Yu]

+++ Andi Kleen [24/01/18 10:17 -0800]:

On Wed, Jan 24, 2018 at 09:00:48AM -0800, Linus Torvalds wrote:

On Wed, Jan 24, 2018 at 6:28 AM, Greg Kroah-Hartman
 wrote:
>
> Linus, if there are no objections, can you apply this revert to your
> tree now so this doesn't get into 4.15?

Applied.


So can we get the warning replacement? It would be good to have some
kind of solution.

-Andi



retpoline/module: Warn for missing retpoline in module

There's a risk that a kernel that has full retpoline mitigations
becomes vulnerable when a module gets loaded that hasn't been
compiled with the right compiler or the right option.

We cannot fix it, but should at least warn the user when that
happens.

When the a module hasn't been compiled with a retpoline
aware compiler, print a warning and change the SPECTRE_V2
mitigation mode to show the system is vulnerable now.

For modules it is checked at compile time, however it cannot
check assembler or other non compiled objects used in the module link.

v2: Change warning message
v3: Port to latest tree
v4: Remove tainting


So I thought distros wanted the module taint after all, as Greg
mentioned, or is that still overkill? Would the printed warning
be sufficient for the distro folks?


Cc: j...@kernel.org
Signed-off-by: Andi Kleen 
Signed-off-by: David Woodhouse 

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 9c18da64daa9..ea707c91bd8c 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -970,4 +970,8 @@ bool xen_set_default_idle(void);

void stop_this_cpu(void *dummy);
void df_debug(struct pt_regs *regs, long error_code);
+
+void disable_retpoline(void);
+bool retpoline_enabled(void);
+
#endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index e4dc26185aa7..9064b20473a7 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -93,6 +93,18 @@ static const char *spectre_v2_strings[] = {

static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;

+/* A module has been loaded. Disable reporting that we're good. */
+void disable_retpoline(void)
+{
+   spectre_v2_enabled = SPECTRE_V2_NONE;
+   pr_err("system may be vunerable to spectre\n");
+}
+
+bool retpoline_enabled(void)
+{
+   return spectre_v2_enabled != SPECTRE_V2_NONE;
+}
+
static void __init spec2_print_if_insecure(const char *reason)
{
if (boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..136ea6cabec6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3020,7 +3020,13 @@ static int check_modinfo(struct module *mod, struct 
load_info *info, int flags)
mod->name);
add_taint_module(mod, TAINT_OOT_MODULE, LOCKDEP_STILL_OK);
}
-
+#ifdef RETPOLINE
+   if (retpoline_enabled() && !get_modinfo(info, "retpoline")) {
+   pr_warn("%s: loading module not compiled with retpoline 
compiler.\n",
+   mod->name);
+   disable_retpoline();
+   }
+#endif
if (get_modinfo(info, "staging")) {
add_taint_module(mod, TAINT_CRAP, LOCKDEP_STILL_OK);
pr_warn("%s: module is from the staging directory, the quality "
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 98314b400a95..54deaa1066cf 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2165,6 +2165,14 @@ static void add_intree_flag(struct buffer *b, int 
is_intree)
buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n");
}

+/* Cannot check for assembler */
+static void add_retpoline(struct buffer *b)
+{
+   buf_printf(b, "\n#ifdef RETPOLINE\n");
+   buf_printf(b, "MODULE_INFO(retpoline, \"Y\");\n");
+   buf_printf(b, "#endif\n");
+}
+
static void add_staging_flag(struct buffer *b, const char *name)
{
static const char *staging_dir = "drivers/staging";
@@ -2506,6 +2514,7 @@ int main(int argc, char **argv)
err |= check_modname_len(mod);
add_header(&buf, mod);
add_intree_flag(&buf, !external_module);
+   add_retpoline(&buf);
add_staging_flag(&buf, mod->name);
err |= add_versions(&buf, mod);
add_depends(&buf, mod, modules);

Re: [PATCH] powerpc/modules: If mprofile-kernel is enabled add it to vermagic

[Jessica Yu]

+++ Michael Ellerman [10/05/17 16:57 +1000]:

On powerpc we can build the kernel with two different ABIs for mcount(), which
is used by ftrace. Kernels built with one ABI do not know how to load modules
built with the other ABI. The new style ABI is called "mprofile-kernel", for
want of a better name.

Currently if we build a module using the old style ABI, and the kernel with
mprofile-kernel, when we load the module we'll oops something like:

 # insmod autofs4-no-mprofile-kernel.ko
 ftrace-powerpc: Unexpected instruction f8810028 around bl _mcount
 [ cut here ]
 WARNING: CPU: 6 PID: 3759 at ../kernel/trace/ftrace.c:2024 
ftrace_bug+0x2b8/0x3c0
 CPU: 6 PID: 3759 Comm: insmod Not tainted 
4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269 #11
 ...
 NIP [c01eaa48] ftrace_bug+0x2b8/0x3c0
 LR [c01eaff8] ftrace_process_locs+0x4a8/0x590
 Call Trace:
   alloc_pages_current+0xc4/0x1d0 (unreliable)
   ftrace_process_locs+0x4a8/0x590
   load_module+0x1c8c/0x28f0
   SyS_finit_module+0x110/0x140
   system_call+0x38/0xfc
 ...
 ftrace failed to modify
 [] 0xd2a31024
  actual:   35:65:00:48

We can avoid this by including in the vermagic whether the kernel/module was
built with mprofile-kernel. Which results in:

 # insmod autofs4-pg.ko
 autofs4: version magic
 '4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269 SMP mod_unload modversions '
 should be
 '4.11.0-rc3-gcc-5.4.1-00017-g5a61ef74f269-dirty SMP mod_unload modversions 
mprofile-kernel'
 insmod: ERROR: could not insert module autofs4-pg.ko: Invalid module format

Signed-off-by: Michael Ellerman 


Looks good to me:

Acked-by: Jessica Yu 


---
arch/powerpc/include/asm/module.h | 4 
1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/include/asm/module.h 
b/arch/powerpc/include/asm/module.h
index 53885512b8d3..6c0132c7212f 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -14,6 +14,10 @@
#include 


+#ifdef CC_USING_MPROFILE_KERNEL
+#define MODULE_ARCH_VERMAGIC   "mprofile-kernel"
+#endif
+
#ifndef __powerpc64__
/*
 * Thanks to Paul M for explaining this.
--
2.7.4

Re: [PATCH v3] ftrace/module: Move ftrace_release_mod() to ddebug_cleanup label

[Jessica Yu]

+++ Namit Gupta [08/01/18 10:41 +0530]:

ftrace_module_init happen after dynamic_debug_setup, it is desired that
cleanup should be called after this label however in current implementation
it is called in free module label,ie:even though ftrace in not initialized,
from so many fail case ftrace_release_mod() will be called and unnecessary
traverse the whole list.
In below patch we moved ftrace_release_mod() from free_module label to
ddebug_cleanup label. that is the best possible location, other solution
is to make new label to ftrace_release_mod() but since ftrace_module_init()
is not return with minimum changes it should be in ddebug_cleanup label.

Signed-off-by: Namit Gupta 


Hi Steven,

Could I get an ACK for this patch?

Thanks!

Jessica

[PATCH v4 0/2] kprobes: improve error handling when arming/disarming kprobes

[Jessica Yu]

Hi,

This patchset attempts to improve error handling when arming or disarming
ftrace-based kprobes. The current behavior is to simply WARN when ftrace
(un-)registration fails, without propagating the error code. This can lead
to confusing situations where, for example, register_kprobe()/enable_kprobe()
would return 0 indicating success even if arming via ftrace had failed. In
this scenario we'd end up with a non-functioning kprobe even though kprobe
registration (or enablement) returned success. In this patchset, we take
errors from ftrace into account and propagate the error when we cannot arm
or disarm a kprobe.

Below is an example that illustrates the problem using livepatch and
systemtap (which uses kprobes underneath). Both livepatch and kprobes use
ftrace ops with the IPMODIFY flag set, so registration at the same
function entry is limited to only one ftrace user. 

Before
--
# modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has 
IPMODIFY set
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'

   .. (nothing prints after reading /proc/cmdline) ..

The systemtap handler doesn't execute due to a kprobe arming failure caused
by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
indication of error from systemtap (because register_kprobe() returned
success) unless the user inspects dmesg.

After
-
# modprobe livepatch-sample 
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'
WARNING: probe 
kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call
 (address 0xa82fe910) registration error (rc -16)

Although the systemtap handler doesn't execute (as it shouldn't), the
ftrace error is propagated and now systemtap prints a visible error message
stating that (kprobe) registration had failed (because register_kprobe()
returned an error), along with the propagated error code.

This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

This patchset has been lightly sanity-tested (on linux-next) with kprobes,
kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
more testing is greatly appreciated.

Changes from v3:
 - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
   !CONFIG_KPROBES_ON_FTRACE
 - Add total count of all probes tried in (dis)arm_all_kprobes()

Changes from v2:
 - Add missing synchronize rcu in register_aggr_kprobe()
 - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()

Changes from v1:
- Don't arm the kprobe before adding it to the kprobe table, otherwise
  we'll temporarily see a stray breakpoint.
- Remove kprobe from the kprobe_table and call synchronize_sched() if
  arming during register_kprobe() fails.
- add Masami's ack on the 2nd patch (unchanged from v1)

---
Jessica Yu (2):
  kprobes: propagate error from arm_kprobe_ftrace()
  kprobes: propagate error from disarm_kprobe_ftrace()

 kernel/kprobes.c | 174 +++
 1 file changed, 124 insertions(+), 50 deletions(-)

-- 
2.13.6

[PATCH v4 2/2] kprobes: propagate error from disarm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.

disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all probes could be disarmed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Acked-by: Masami Hiramatsu 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 78 ++--
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 21d88cebb29b..2792450b811e 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1017,23 +1017,27 @@ static int arm_kprobe_ftrace(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
-   kprobe_ftrace_enabled--;
-   if (kprobe_ftrace_enabled == 0) {
+   if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", 
ret))
+   return ret;
}
+
+   kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, 
ret);
+   return ret;
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)   (-ENODEV)
-#define disarm_kprobe_ftrace(p)do {} while (0)
+#define disarm_kprobe_ftrace(p)(-ENODEV)
 #endif
 
 /* Arm a kprobe with text_mutex */
@@ -1052,18 +1056,18 @@ static int arm_kprobe(struct kprobe *kp)
 }
 
 /* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   disarm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return disarm_kprobe_ftrace(kp);
 
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /*
@@ -1656,11 +1660,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 static struct kprobe *__disable_kprobe(struct kprobe *p)
 {
struct kprobe *orig_p;
+   int ret;
 
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
-   return NULL;
+   return ERR_PTR(-EINVAL);
 
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1674,8 +1679,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
 * should have already been disarmed, so
 * skip unneed disarming process.
 */
-   if (!kprobes_all_disarmed)
-   disarm_kprobe(orig_p, true);
+   if (!kprobes_all_disarmed) {
+   ret = disarm_kprobe(orig_p, true);
+   if (ret) {
+   p->flags &= ~KPROBE_FLAG_DISABLED;
+   return ERR_PTR(ret);
+   }
+   }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1692,8 +1702,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
 
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
-   if (ap == NULL)
-   return -EINVAL;
+   if (IS_ERR(ap))
+   return PTR_ERR(ap);
 
if (ap == p)
/*
@@ -2126,12 +2136,14 @@ static void kill_kprobe(struct kprobe *p)
 int disable_kprobe(struct kprobe *kp)
 {
int ret = 0;
+   struct kprobe *p;
 
mutex_lock(&kprobe_mutex);
 
/* Disable this kprobe */
-   if (__disable_kprobe(kp) == NULL)
-   ret = -EINVAL;
+   p = __disable_kprobe(kp);
+   if (I

[PATCH v4 1/2] kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.

arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all probes could be armed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 96 +---
 1 file changed, 71 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b4aab48ad258..21d88cebb29b 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -988,18 +988,32 @@ static int prepare_kprobe(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 0, 0);
-   WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
-   kprobe_ftrace_enabled++;
-   if (kprobe_ftrace_enabled == 1) {
+   if (WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, 
ret))
+   return ret;
+
+   if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret))
+   goto err_ftrace;
}
+
+   kprobe_ftrace_enabled++;
+   return ret;
+
+err_ftrace:
+   /*
+* Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
+* non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
+* empty filter_hash which would undesirably trace all functions.
+*/
+   ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+   return ret;
 }
 
 /* Caller must lock kprobe_mutex */
@@ -1018,22 +1032,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p)   do {} while (0)
+#define arm_kprobe_ftrace(p)   (-ENODEV)
 #define disarm_kprobe_ftrace(p)do {} while (0)
 #endif
 
 /* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   arm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /* Disarm a kprobe with text_mutex */
@@ -1372,9 +1387,15 @@ static int register_aggr_kprobe(struct kprobe *orig_p, 
struct kprobe *p)
 
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
-   if (!kprobes_all_disarmed)
+   if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
-   arm_kprobe(ap);
+   ret = arm_kprobe(ap);
+   if (ret) {
+   ap->flags |= KPROBE_FLAG_DISABLED;
+   list_del_rcu(&p->list);
+   synchronize_sched();
+   }
+   }
}
return ret;
 }
@@ -1594,8 +1615,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
   &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
 
-   if (!kprobes_all_disarmed &&am

Re: [PATCH] modpost: Remove trailing semicolon

[Jessica Yu]

+++ Luis de Bethencourt [16/01/18 13:21 +]:

The trailing semicolon is an empty statement that does no operation.
Removing it since it doesn't do anything.

Signed-off-by: Luis de Bethencourt 


Applied. Thanks,

Jessica

[PATCH v5 0/2] kprobes: improve error handling when arming/disarming kprobes

[Jessica Yu]

Hi,

This patchset attempts to improve error handling when arming or disarming
ftrace-based kprobes. The current behavior is to simply WARN when ftrace
(un-)registration fails, without propagating the error code. This can lead
to confusing situations where, for example, register_kprobe()/enable_kprobe()
would return 0 indicating success even if arming via ftrace had failed. In
this scenario we'd end up with a non-functioning kprobe even though kprobe
registration (or enablement) returned success. In this patchset, we take
errors from ftrace into account and propagate the error when we cannot arm
or disarm a kprobe.

Below is an example that illustrates the problem using livepatch and
systemtap (which uses kprobes underneath). Both livepatch and kprobes use
ftrace ops with the IPMODIFY flag set, so registration at the same
function entry is limited to only one ftrace user. 

Before
--
# modprobe livepatch-sample # patches cmdline_proc_show, ftrace ops has 
IPMODIFY set
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'

   .. (nothing prints after reading /proc/cmdline) ..

The systemtap handler doesn't execute due to a kprobe arming failure caused
by a ftrace IPMODIFY conflict with livepatch, and there isn't an obvious
indication of error from systemtap (because register_kprobe() returned
success) unless the user inspects dmesg.

After
-
# modprobe livepatch-sample 
# stap -e 'probe kernel.function("cmdline_proc_show").call { printf 
("cmdline_proc_show\n"); }'
WARNING: probe 
kernel.function("cmdline_proc_show@/home/jeyu/work/linux-next/fs/proc/cmdline.c:6").call
 (address 0xa82fe910) registration error (rc -16)

Although the systemtap handler doesn't execute (as it shouldn't), the
ftrace error is propagated and now systemtap prints a visible error message
stating that (kprobe) registration had failed (because register_kprobe()
returned an error), along with the propagated error code.

This patchset was based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

This patchset has been lightly sanity-tested (on linux-next) with kprobes,
kretprobes, and optimized kprobes. It passes the kprobes smoke test, but
more testing is greatly appreciated.

Changes from v4:
 - Switch from WARN() to pr_debug() in arm_kprobe_ftrace() so the stack
   dumps don't pollute dmesg, as IPMODIFY conflicts can occur in normal usage
 - Added Masami's ack to the first patch

Changes from v3:
 - Have (dis)arm_kprobe_ftrace() return -ENODEV instead of 0 in case of
   !CONFIG_KPROBES_ON_FTRACE
 - Add total count of all probes tried in (dis)arm_all_kprobes()

Changes from v2:
 - Add missing synchronize rcu in register_aggr_kprobe()
 - s/kprobes/probes/ on error message in (dis)arm_all_kprobes()

Changes from v1:
- Don't arm the kprobe before adding it to the kprobe table, otherwise
  we'll temporarily see a stray breakpoint.
- Remove kprobe from the kprobe_table and call synchronize_sched() if
  arming during register_kprobe() fails.
- add Masami's ack on the 2nd patch (unchanged from v1)

---
Jessica Yu (2):
  kprobes: propagate error from arm_kprobe_ftrace()
  kprobes: propagate error from disarm_kprobe_ftrace()

 kernel/kprobes.c | 178 +++
 1 file changed, 128 insertions(+), 50 deletions(-)

-- 
2.13.6

[PATCH v5 1/2] kprobes: propagate error from arm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when arming ftrace-based kprobes. Specifically, if
we fail to arm a ftrace-based kprobe, register_kprobe()/enable_kprobe()
should report an error instead of success. Previously, this has lead to
confusing situations where register_kprobe() would return 0 indicating
success, but the kprobe would not be functional if ftrace registration
during the kprobe arming process had failed. We should therefore take any
errors returned by ftrace into account and propagate this error so that we
do not register/enable kprobes that cannot be armed. This can happen if,
for example, register_ftrace_function() finds an IPMODIFY conflict (since
kprobe_ftrace_ops has this flag set) and returns an error. Such a conflict
is possible since livepatches also set the IPMODIFY flag for their ftrace_ops.

arm_all_kprobes() keeps its current behavior and attempts to arm all
kprobes. It returns the last encountered error and gives a warning if
not all probes could be armed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Acked-by: Masami Hiramatsu 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 100 +--
 1 file changed, 75 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b4aab48ad258..beda29641ed5 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -988,18 +988,36 @@ static int prepare_kprobe(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void arm_kprobe_ftrace(struct kprobe *p)
+static int arm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 0, 0);
-   WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
-   kprobe_ftrace_enabled++;
-   if (kprobe_ftrace_enabled == 1) {
+   if (ret) {
+   pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, 
ret);
+   return ret;
+   }
+
+   if (kprobe_ftrace_enabled == 0) {
ret = register_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (ret) {
+   pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
+   goto err_ftrace;
+   }
}
+
+   kprobe_ftrace_enabled++;
+   return ret;
+
+err_ftrace:
+   /*
+* Note: Since kprobe_ftrace_ops has IPMODIFY set, and ftrace requires a
+* non-empty filter_hash for IPMODIFY ops, we're safe from an accidental
+* empty filter_hash which would undesirably trace all functions.
+*/
+   ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
+   return ret;
 }
 
 /* Caller must lock kprobe_mutex */
@@ -1018,22 +1036,23 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
-#define arm_kprobe_ftrace(p)   do {} while (0)
+#define arm_kprobe_ftrace(p)   (-ENODEV)
 #define disarm_kprobe_ftrace(p)do {} while (0)
 #endif
 
 /* Arm a kprobe with text_mutex */
-static void arm_kprobe(struct kprobe *kp)
+static int arm_kprobe(struct kprobe *kp)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   arm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return arm_kprobe_ftrace(kp);
+
cpus_read_lock();
mutex_lock(&text_mutex);
__arm_kprobe(kp);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /* Disarm a kprobe with text_mutex */
@@ -1372,9 +1391,15 @@ static int register_aggr_kprobe(struct kprobe *orig_p, 
struct kprobe *p)
 
if (ret == 0 && kprobe_disabled(ap) && !kprobe_disabled(p)) {
ap->flags &= ~KPROBE_FLAG_DISABLED;
-   if (!kprobes_all_disarmed)
+   if (!kprobes_all_disarmed) {
/* Arm the breakpoint again. */
-   arm_kprobe(ap);
+   ret = arm_kprobe(ap);
+   if (ret) {
+   ap->flags |= KPROBE_FLAG_DISABLED;
+   list_del_rcu(&p->list);
+   synchronize_sched();
+   }
+   }
}
return ret;
 }
@@ -1594,8 +1619,14 @@ int register_kprobe(struct kprobe *p)
hlist_add_head_rcu(&p->hlist,
   &kprobe_t

[PATCH v5 2/2] kprobes: propagate error from disarm_kprobe_ftrace()

[Jessica Yu]

Improve error handling when disarming ftrace-based kprobes. Like with
arm_kprobe_ftrace(), propagate any errors from disarm_kprobe_ftrace() so
that we do not disable/unregister kprobes that are still armed. In other
words, unregister_kprobe() and disable_kprobe() should not report success
if the kprobe could not be disarmed.

disarm_all_kprobes() keeps its current behavior and attempts to
disarm all kprobes. It returns the last encountered error and gives a
warning if not all probes could be disarmed.

This patch is based on Petr Mladek's original patchset (patches 2 and 3)
back in 2015, which improved kprobes error handling, found here:

   https://lkml.org/lkml/2015/2/26/452

However, further work on this had been paused since then and the patches
were not upstreamed.

Based-on-patches-by: Petr Mladek 
Acked-by: Masami Hiramatsu 
Signed-off-by: Jessica Yu 
---
 kernel/kprobes.c | 78 ++--
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index beda29641ed5..0951d6f0d59d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1021,23 +1021,27 @@ static int arm_kprobe_ftrace(struct kprobe *p)
 }
 
 /* Caller must lock kprobe_mutex */
-static void disarm_kprobe_ftrace(struct kprobe *p)
+static int disarm_kprobe_ftrace(struct kprobe *p)
 {
-   int ret;
+   int ret = 0;
 
-   kprobe_ftrace_enabled--;
-   if (kprobe_ftrace_enabled == 0) {
+   if (kprobe_ftrace_enabled == 1) {
ret = unregister_ftrace_function(&kprobe_ftrace_ops);
-   WARN(ret < 0, "Failed to init kprobe-ftrace (%d)\n", ret);
+   if (WARN(ret < 0, "Failed to unregister kprobe-ftrace (%d)\n", 
ret))
+   return ret;
}
+
+   kprobe_ftrace_enabled--;
+
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
   (unsigned long)p->addr, 1, 0);
WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, 
ret);
+   return ret;
 }
 #else  /* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)  arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)   (-ENODEV)
-#define disarm_kprobe_ftrace(p)do {} while (0)
+#define disarm_kprobe_ftrace(p)(-ENODEV)
 #endif
 
 /* Arm a kprobe with text_mutex */
@@ -1056,18 +1060,18 @@ static int arm_kprobe(struct kprobe *kp)
 }
 
 /* Disarm a kprobe with text_mutex */
-static void disarm_kprobe(struct kprobe *kp, bool reopt)
+static int disarm_kprobe(struct kprobe *kp, bool reopt)
 {
-   if (unlikely(kprobe_ftrace(kp))) {
-   disarm_kprobe_ftrace(kp);
-   return;
-   }
+   if (unlikely(kprobe_ftrace(kp)))
+   return disarm_kprobe_ftrace(kp);
 
cpus_read_lock();
mutex_lock(&text_mutex);
__disarm_kprobe(kp, reopt);
mutex_unlock(&text_mutex);
cpus_read_unlock();
+
+   return 0;
 }
 
 /*
@@ -1660,11 +1664,12 @@ static int aggr_kprobe_disabled(struct kprobe *ap)
 static struct kprobe *__disable_kprobe(struct kprobe *p)
 {
struct kprobe *orig_p;
+   int ret;
 
/* Get an original kprobe for return */
orig_p = __get_valid_kprobe(p);
if (unlikely(orig_p == NULL))
-   return NULL;
+   return ERR_PTR(-EINVAL);
 
if (!kprobe_disabled(p)) {
/* Disable probe if it is a child probe */
@@ -1678,8 +1683,13 @@ static struct kprobe *__disable_kprobe(struct kprobe *p)
 * should have already been disarmed, so
 * skip unneed disarming process.
 */
-   if (!kprobes_all_disarmed)
-   disarm_kprobe(orig_p, true);
+   if (!kprobes_all_disarmed) {
+   ret = disarm_kprobe(orig_p, true);
+   if (ret) {
+   p->flags &= ~KPROBE_FLAG_DISABLED;
+   return ERR_PTR(ret);
+   }
+   }
orig_p->flags |= KPROBE_FLAG_DISABLED;
}
}
@@ -1696,8 +1706,8 @@ static int __unregister_kprobe_top(struct kprobe *p)
 
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
-   if (ap == NULL)
-   return -EINVAL;
+   if (IS_ERR(ap))
+   return PTR_ERR(ap);
 
if (ap == p)
/*
@@ -2130,12 +2140,14 @@ static void kill_kprobe(struct kprobe *p)
 int disable_kprobe(struct kprobe *kp)
 {
int ret = 0;
+   struct kprobe *p;
 
mutex_lock(&kprobe_mutex);
 
/* Disable this kprobe */
-   if (__disable_kprobe(kp) == NULL)
-   ret = -EINVAL;
+   p = __disable_kprobe(kp);
+   if (I

Re: [PATCH v2 0/4] modsign enhancement

[Jessica Yu]

+++ Jia Zhang [08/03/18 12:26 +0800]:

This patch series allows to disable module validity enforcement
in runtime through /sys/kernel/security/modsign/enforce interface.

Assuming CONFIG_MODULE_SIG_FORCE=y, here are the instructions to
disable the validity enforcement.

# cat /sys/kernel/security/modsign/enforce
# echo -n 0 > data
# openssl smime -sign -nocerts -noattr -binary -in data \
   -inkey  -signer  -outform der \
   -out /sys/kernel/security/modsign/enforce

Now enable enforcement again on demand.

# echo 1 > /sys/kernel/security/modsign/enforce

Changelog:
v2:
- Support to disable validity enforcement in runtime.


NAK - please use /sys/module/module/parameters/sig_enforce.

And I would rather keep this parameter bool_enable_only, plain and simple.
What use case do you have/why would you want to disable signature
enforcement - after having enabled it - during runtime?  None of this
is explained nor justified in the cover letter.

Thanks,

Jessica

Re: [PATCH 3/4] module: Support to show the current enforcement policy

[Jessica Yu]

+++ Jia Zhang [01/03/18 17:09 +0800]:

/sys/kernel/security/modsign/enforce gives the result of current
enforcement policy of loading module.

Signed-off-by: Jia Zhang 


Why is this being added as part of securityfs? AFAIK that's primarily used by 
LSMs.

And we already export sig_enforce to sysfs (See 
/sys/module/module/parameters/sig_enforce).
It already does exactly what your patchset tries to do, it only allows for enablement. 


Jessica


---
kernel/module.c | 55 +++
1 file changed, 55 insertions(+)

diff --git a/kernel/module.c b/kernel/module.c
index 79825ea..e3c6c8e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2794,11 +2794,60 @@ static int module_sig_check(struct load_info *info, int 
flags)

return err;
}
+
+#ifdef CONFIG_SECURITYFS
+static ssize_t modsign_enforce_read(struct file *filp, char __user *ubuf,
+   size_t count, loff_t *offp)
+{
+   char buf[2];
+
+   sprintf(buf, "%d", !!sig_enforce);
+
+   return simple_read_from_buffer(ubuf, count, offp, buf, 1);
+}
+
+static const struct file_operations modsign_enforce_ops = {
+   .read = modsign_enforce_read,
+   .llseek = generic_file_llseek,
+};
+
+static int __init securityfs_init(void)
+{
+   struct dentry *modsign_dir;
+   struct dentry *enforce;
+
+   modsign_dir = securityfs_create_dir("modsign", NULL);
+   if (IS_ERR(modsign_dir))
+   return -1;
+
+   enforce = securityfs_create_file("enforce",
+S_IRUSR | S_IRGRP, modsign_dir,
+NULL, &modsign_enforce_ops);
+   if (IS_ERR(enforce))
+   goto out;
+
+   return 0;
+out:
+   securityfs_remove(modsign_dir);
+
+   return -1;
+}
+#else /* !CONFIG_SECURITYFS */
+static int __init securityfs_init(void)
+{
+   return 0;
+}
+#endif
#else /* !CONFIG_MODULE_SIG */
static int module_sig_check(struct load_info *info, int flags)
{
return 0;
}
+
+static int __init securityfs_init(void)
+{
+   return 0;
+}
#endif /* !CONFIG_MODULE_SIG */

/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
@@ -4395,8 +,14 @@ void module_layout(struct module *mod,

static int __init initialize_module(void)
{
+   int ret;
+
proc_modules_init();

+   ret = securityfs_init();
+   if (unlikely(ret))
+   return ret;
+
return 0;
}
module_init(initialize_module);
--
1.8.3.1

[GIT PULL] Modules fix for 4.16-rc7

[Jessica Yu]

The following changes since commit 661e50bc853209e41a5c14a290ca4decc43cbfd1:

 Linux 4.16-rc4 (2018-03-04 14:54:11 -0800)

are available in the git repository at:

 ssh://g...@gitolite.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git 
tags/modules-for-v4.16-rc7

for you to fetch changes up to 3f553b308bb004eb730da8e00a28150c157c7724:

 module: propagate error in modules_open() (2018-03-08 21:58:51 +0100)


Modules fix for v4.16-rc7

- Propagate error in modules_open() to avoid possible later NULL
 dereference if seq_open() had failed.

Signed-off-by: Jessica Yu 


Leon Yu (1):
 module: propagate error in modules_open()

kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH resend] module: propagate error in modules_open()

[Jessica Yu]

+++ Leon Yu [06/03/18 23:16 +0800]:

otherwise kernel can oops later in seq_release() due to dereferencing null
file->private_data which is only set if seq_open() succeeds.

BUG: unable to handle kernel NULL pointer dereference at 
IP: seq_release+0xc/0x30
Call Trace:
close_pdeo+0x37/0xd0
proc_reg_release+0x5d/0x60
__fput+0x9d/0x1d0
fput+0x9/0x10
task_work_run+0x75/0x90
do_exit+0x252/0xa00
do_group_exit+0x36/0xb0
SyS_exit_group+0xf/0x10

Fixes: 516fb7f2e73d ("/proc/module: use the same logic as /proc/kallsyms for address 
exposure")
Cc: Jessica Yu 
Cc: Linus Torvalds 
Cc: sta...@vger.kernel.org # 4.15+
Signed-off-by: Leon Yu 


Ah yeah, the error code was dropped in that commit. 


Applied, thanks for the fix.

Jessica


---
kernel/module.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ad2d420024f6..e42764acedb4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4228,7 +4228,7 @@ static int modules_open(struct inode *inode, struct file 
*file)
m->private = kallsyms_show_value() ? NULL : (void *)8ul;
}

-   return 0;
+   return err;
}

static const struct file_operations proc_modules_operations = {
--
2.16.2

Re: linux-next: Signed-off-by missing for commit in the modules tree

[Jessica Yu]

+++ Stephen Rothwell [09/03/18 07:54 +1100]:

Hi Jessica,

Commit

 934ccf7248dd ("module: propagate error in modules_open()")

is missing a Signed-off-by from its committer.



Whoops, thanks for catching that. Should be fixed now.

Jessica

[PATCH v2] module: make it clearer when we're handling kallsyms symbols vs exported symbols

[Jessica Yu]

The module loader internally works with both exported symbols
represented as struct kernel_symbol, as well as Elf symbols from a
module's symbol table. It's hard to distinguish sometimes which type of
symbol we're handling given that some helper function names are not
consistent or helpful. Take get_ksymbol() for instance - are we
looking for an exported symbol or a kallsyms symbol here? Or symname()
and kernel_symbol_name() - which function handles an exported symbol and
which one an Elf symbol?

Clean up and unify the function naming scheme a bit to make it clear
which kind of symbol we're handling. This change only affects static
functions internal to the module loader.

Signed-off-by: Jessica Yu 
---

v2: renamed kallsyms_find_* funcs to find_kallsyms_* to follow the
already existing _ naming convention in module.c

 kernel/module.c | 78 -
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..1b5edf78694c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -495,9 +495,9 @@ struct find_symbol_arg {
const struct kernel_symbol *sym;
 };
 
-static bool check_symbol(const struct symsearch *syms,
-struct module *owner,
-unsigned int symnum, void *data)
+static bool check_exported_symbol(const struct symsearch *syms,
+ struct module *owner,
+ unsigned int symnum, void *data)
 {
struct find_symbol_arg *fsa = data;
 
@@ -555,9 +555,9 @@ static int cmp_name(const void *va, const void *vb)
return strcmp(a, kernel_symbol_name(b));
 }
 
-static bool find_symbol_in_section(const struct symsearch *syms,
-  struct module *owner,
-  void *data)
+static bool find_exported_symbol_in_section(const struct symsearch *syms,
+   struct module *owner,
+   void *data)
 {
struct find_symbol_arg *fsa = data;
struct kernel_symbol *sym;
@@ -565,13 +565,14 @@ static bool find_symbol_in_section(const struct symsearch 
*syms,
sym = bsearch(fsa->name, syms->start, syms->stop - syms->start,
sizeof(struct kernel_symbol), cmp_name);
 
-   if (sym != NULL && check_symbol(syms, owner, sym - syms->start, data))
+   if (sym != NULL && check_exported_symbol(syms, owner,
+sym - syms->start, data))
return true;
 
return false;
 }
 
-/* Find a symbol and return it, along with, (optional) crc and
+/* Find an exported symbol and return it, along with, (optional) crc and
  * (optional) module which owns it.  Needs preempt disabled or module_mutex. */
 const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
@@ -585,7 +586,7 @@ const struct kernel_symbol *find_symbol(const char *name,
fsa.gplok = gplok;
fsa.warn = warn;
 
-   if (each_symbol_section(find_symbol_in_section, &fsa)) {
+   if (each_symbol_section(find_exported_symbol_in_section, &fsa)) {
if (owner)
*owner = fsa.owner;
if (crc)
@@ -2198,7 +2199,7 @@ EXPORT_SYMBOL_GPL(__symbol_get);
  *
  * You must hold the module_mutex.
  */
-static int verify_export_symbols(struct module *mod)
+static int verify_exported_symbols(struct module *mod)
 {
unsigned int i;
struct module *owner;
@@ -2519,10 +2520,10 @@ static void free_modinfo(struct module *mod)
 
 #ifdef CONFIG_KALLSYMS
 
-/* lookup symbol in given range of kernel_symbols */
-static const struct kernel_symbol *lookup_symbol(const char *name,
-   const struct kernel_symbol *start,
-   const struct kernel_symbol *stop)
+/* Lookup exported symbol in given range of kernel_symbols */
+static const struct kernel_symbol *lookup_exported_symbol(const char *name,
+ const struct 
kernel_symbol *start,
+ const struct 
kernel_symbol *stop)
 {
return bsearch(name, start, stop - start,
sizeof(struct kernel_symbol), cmp_name);
@@ -2533,9 +2534,10 @@ static int is_exported(const char *name, unsigned long 
value,
 {
const struct kernel_symbol *ks;
if (!mod)
-   ks = lookup_symbol(name, __start___ksymtab, __stop___ksymtab);
+   ks = lookup_exported_symbol(name, __start___ksymtab, 
__stop___ksymtab);
else
-   ks = lookup_symbol(name, mod->syms, mod->syms + mod->num_syms);
+   ks = lookup_exported_symbol(name, mod->syms, mod->syms + 
mod->num_syms);
+
  

Re: [PATCH v9 RESEND 0/4] KASLR feature to randomize each loadable module

[Jessica Yu]

+++ Rick Edgecombe [20/11/18 15:23 -0800]:

Resending this because I missed Jessica in the "to" list. Also removing the part
of this coverletter that talked about KPTI helping with some local kernel text
de-randomizing methods, because I'm not sure I fully understand this.



This is V9 of the "KASLR feature to randomize each loadable module" patchset.
The purpose is to increase the randomization for the module space from 10 to 17
bits, and also to make the modules randomized in relation to each other instead
of just the address where the allocations begin, so that if one module leaks the
location of the others can't be inferred.

Why its useful
==
Randomizing the location of executable code is a defense against control flow
attacks, where the kernel is tricked into jumping, or speculatively executing
code other than what is intended. By randomizing the location of the code, the
attacker doesn't know where to redirect the control flow.

Today the RANDOMIZE_BASE feature randomizes the base address where the module
allocations begin with 10 bits of entropy for this purpose. From here, a highly
deterministic algorithm allocates space for the modules as they are loaded and
unloaded. If an attacker can predict the order and identities for modules that
will be loaded (either by the system, or controlled by the user with
request_module or BPF), then a single text address leak can give the attacker
access to the locations of other modules. So in this case this new algorithm can
take the entropy of the other modules from ~0 to 17, making it much more robust.

Another problem today is that the low 10 bits of entropy makes brute force
attacks feasible, especially in the case of speculative execution where a wrong
guess won't necessarily cause a crash. In this case, increasing the
randomization will force attacks to take longer, and so increase the time an
attacker may be detected on a system.

There are multiple efforts to apply more randomization to the core kernel text
as well, and so this module space piece can be a first step to increasing
randomization for all kernel space executable code.

Userspace ASLR can get 28 bits of entropy or more, so at least increasing this
to 17 for now improves what is currently a pretty low amount of randomization
for the higher privileged kernel space.

How it works

The algorithm is pretty simple. It just breaks the module space in two, a random
area (2/3 of module space) and a backup area (1/3 of module space). It first
tries to allocate up to 1 randomly located starting pages inside the random
section. If this fails, it will allocate in the backup area. The backup area
base will be offset in the same way as current algorithm does for the base area,
which has 10 bits of entropy.

The vmalloc allocator can be used to try an allocation at a specific address,
however it is usually used to try an allocation over a large address range, and
so some behaviors which are non-issues in normal usage can be be sub-optimal
when trying the an allocation at 1 small ranges. So this patch also includes
a new vmalloc function __vmalloc_node_try_addr and some other vmalloc tweaks
that allow for more efficient trying of addresses.

This algorithm targets maintaining high entropy for many 1000's of module
allocations. This is because there are other users of the module space besides
kernel modules, like eBPF JIT, classic BPF socket filter JIT and kprobes.


Hi Rick!

Sorry for the delay. I'd like to take a step back and ask some broader 
questions -

- Is the end goal of this patchset to randomize loading kernel modules, or 
most/all
  executable kernel memory allocations, including bpf, kprobes, etc?

- It seems that a lot of complexity and heuristics are introduced just to
  accommodate the potential fragmentation that can happen when the module 
vmalloc
  space starts to get fragmented with bpf filters. I'm partial to the idea of
  splitting or having bpf own its own vmalloc space, similar to what Ard is 
already
  implementing for arm64.

  So a question for the bpf and x86 folks, is having a dedicated vmalloc region
  (as well as a seperate bpf_alloc api) for bpf feasible or desirable on x86_64?

  If bpf filters need to be within 2 GB of the core kernel, would it make sense
  to carve out a portion of the current module region for bpf filters?  
According
  to Documentation/x86/x86_64/mm.txt, the module region is ~1.5 GB. I am 
doubtful
  that any real system will actually have 1.5 GB worth of kernel modules loaded.
  Is there a specific reason why that much space is dedicated to kernel modules,
  and would it be feasible to split that region cleanly with bpf?

- If bpf gets its own dedicated vmalloc space, and we stick to the single task
  of randomizing *just* kernel modules, could the vmalloc optimizations and the
  "backup" area be dropped? The benefits of the vmalloc optimizations seem to
  only b

Re: [PATCH] module: remove some duplicated includes

[Jessica Yu]

+++ Yangtao Li [26/11/18 09:21 -0500]:

We include elf.h twice in module.c. It's unnecessary.
hence just remove them.

Signed-off-by: Yangtao Li 
---
kernel/module.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..438641fc4096 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -28,7 +28,6 @@
#include 
#include 
#include 
-#include 
#include 
#include 
#include 


elf.h is not _directly_ included twice in module.c, although it is
included by other header files that module.c pulls in. I'd rather keep
things as-is, since we shouldn't depend on other header files to pull
in the headers we want. I think it's better to keep the explicit
dependence/inclusion of elf.h in module.c.

Thanks,

Jessica

[PATCH] module: make it clearer when we're handling kallsyms symbols vs exported symbols

[Jessica Yu]

The module loader internally works with both exported symbols
represented as struct kernel_symbol, as well as Elf symbols from a
module's symbol table. It's hard to distinguish sometimes which type of
symbol we're handling given that some helper function names are not
consistent or helpful. Take get_ksymbol() for instance - are we
looking for an exported symbol or a kallsyms symbol here? Or symname()
and kernel_symbol_name() - which function handles an exported symbol and
which one an Elf symbol?

Clean up and unify the function naming scheme a bit to make it clear
which kind of symbol we're handling. This change only affects static
functions internal to the module loader.

Signed-off-by: Jessica Yu 
---
 kernel/module.c | 78 -
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..27d970361e3e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -495,9 +495,9 @@ struct find_symbol_arg {
const struct kernel_symbol *sym;
 };
 
-static bool check_symbol(const struct symsearch *syms,
-struct module *owner,
-unsigned int symnum, void *data)
+static bool check_exported_symbol(const struct symsearch *syms,
+ struct module *owner,
+ unsigned int symnum, void *data)
 {
struct find_symbol_arg *fsa = data;
 
@@ -555,9 +555,9 @@ static int cmp_name(const void *va, const void *vb)
return strcmp(a, kernel_symbol_name(b));
 }
 
-static bool find_symbol_in_section(const struct symsearch *syms,
-  struct module *owner,
-  void *data)
+static bool find_exported_symbol_in_section(const struct symsearch *syms,
+   struct module *owner,
+   void *data)
 {
struct find_symbol_arg *fsa = data;
struct kernel_symbol *sym;
@@ -565,13 +565,14 @@ static bool find_symbol_in_section(const struct symsearch 
*syms,
sym = bsearch(fsa->name, syms->start, syms->stop - syms->start,
sizeof(struct kernel_symbol), cmp_name);
 
-   if (sym != NULL && check_symbol(syms, owner, sym - syms->start, data))
+   if (sym != NULL && check_exported_symbol(syms, owner,
+sym - syms->start, data))
return true;
 
return false;
 }
 
-/* Find a symbol and return it, along with, (optional) crc and
+/* Find an exported symbol and return it, along with, (optional) crc and
  * (optional) module which owns it.  Needs preempt disabled or module_mutex. */
 const struct kernel_symbol *find_symbol(const char *name,
struct module **owner,
@@ -585,7 +586,7 @@ const struct kernel_symbol *find_symbol(const char *name,
fsa.gplok = gplok;
fsa.warn = warn;
 
-   if (each_symbol_section(find_symbol_in_section, &fsa)) {
+   if (each_symbol_section(find_exported_symbol_in_section, &fsa)) {
if (owner)
*owner = fsa.owner;
if (crc)
@@ -2198,7 +2199,7 @@ EXPORT_SYMBOL_GPL(__symbol_get);
  *
  * You must hold the module_mutex.
  */
-static int verify_export_symbols(struct module *mod)
+static int verify_exported_symbols(struct module *mod)
 {
unsigned int i;
struct module *owner;
@@ -2519,10 +2520,10 @@ static void free_modinfo(struct module *mod)
 
 #ifdef CONFIG_KALLSYMS
 
-/* lookup symbol in given range of kernel_symbols */
-static const struct kernel_symbol *lookup_symbol(const char *name,
-   const struct kernel_symbol *start,
-   const struct kernel_symbol *stop)
+/* Lookup exported symbol in given range of kernel_symbols */
+static const struct kernel_symbol *lookup_exported_symbol(const char *name,
+ const struct 
kernel_symbol *start,
+ const struct 
kernel_symbol *stop)
 {
return bsearch(name, start, stop - start,
sizeof(struct kernel_symbol), cmp_name);
@@ -2533,9 +2534,10 @@ static int is_exported(const char *name, unsigned long 
value,
 {
const struct kernel_symbol *ks;
if (!mod)
-   ks = lookup_symbol(name, __start___ksymtab, __stop___ksymtab);
+   ks = lookup_exported_symbol(name, __start___ksymtab, 
__stop___ksymtab);
else
-   ks = lookup_symbol(name, mod->syms, mod->syms + mod->num_syms);
+   ks = lookup_exported_symbol(name, mod->syms, mod->syms + 
mod->num_syms);
+
return ks != NULL && kernel_symbol_value(ks) == value;
 }
 
@@ -3592,7 +3594,7 @@ static int complete

Re: [PATCH] module: make it clearer when we're handling kallsyms symbols vs exported symbols

[Jessica Yu]

+++ Miroslav Benes [22/11/18 11:19 +0100]:

On Wed, 21 Nov 2018, Jessica Yu wrote:


The module loader internally works with both exported symbols
represented as struct kernel_symbol, as well as Elf symbols from a
module's symbol table. It's hard to distinguish sometimes which type of
symbol we're handling given that some helper function names are not
consistent or helpful. Take get_ksymbol() for instance - are we
looking for an exported symbol or a kallsyms symbol here? Or symname()
and kernel_symbol_name() - which function handles an exported symbol and
which one an Elf symbol?

Clean up and unify the function naming scheme a bit to make it clear
which kind of symbol we're handling. This change only affects static
functions internal to the module loader.

Signed-off-by: Jessica Yu 


Great. It should help a lot. Pity we cannot rename find_symbol() as well.

I have only a naming nit. I think it is nice to have
_exported_ convention. New kallsyms_ names don't hold it
though. Wouldn't it be better to be consistent and have
find_kallsyms_symbol() instead of kallsyms_find_symbol()? Or we could do
the opposite and have a "namespace" prefix first. That is,
exported__. However, I don't like it that much.

To be honest, your approach may be the best in the end.

What do you think?


Hi Miroslav, thank you for the comment!

Yeah, that bothered me partially too. And a lot of existing helper
functions in the module loader already have a _ type prefix.

Hm, how about we use _* prefixes if we are just extracting a
value, and _* prefixes for functions actually performing an
action? 


Kallsyms:

  kallsyms_symbol_name()
  kallsyms_symbol_value()
  
  find_kallsyms_symbol()

  find_kallsyms_symbol_value()

  etc.

Exported syms:

  kernel_symbol_name()
  kernel_symbol_value()
  
  lookup_exported_symbol()

  check_exported_symbol()

  etc.

I had left the kernel_symbol_* functions alone since I guess they do
describe what they're handling, they're handling struct kernel_symbol's.
But maybe I will change that to exported_symbol_name() and
exported_symbol_value() to be consistent with the naming scheme..

Thanks!

Jessica

Re: [PATCH 1/2] module: Overwrite st_size instead of st_info

[Jessica Yu]

+++ Vincent Whitchurch [22/11/18 13:24 +0100]:

On Thu, Nov 22, 2018 at 12:01:54PM +, Dave Martin wrote:

On Mon, Nov 19, 2018 at 05:25:12PM +0100, Vincent Whitchurch wrote:
> st_info is currently overwritten after relocation and used to store the
> elf_type().  However, we're going to need it fix kallsyms on ARM's
> Thumb-2 kernels, so preserve st_info and overwrite the st_size field
> instead.  st_size is neither used by the module core nor by any
> architecture.
>
> Signed-off-by: Vincent Whitchurch 
> ---
> v4: Split out to separate patch.  Use st_size instead of st_other.
> v1-v3: See PATCH 2/2
>
>  kernel/module.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 49a405891587..3d86a38b580c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const 
struct load_info *info)
>
>/* Set types up while we still have access to sections. */
>for (i = 0; i < mod->kallsyms->num_symtab; i++)
> -  mod->kallsyms->symtab[i].st_info
> +  mod->kallsyms->symtab[i].st_size
>= elf_type(&mod->kallsyms->symtab[i], info);
>
>/* Now populate the cut down core kallsyms for after init. */
> @@ -4061,7 +4061,7 @@ int module_get_kallsym(unsigned int symnum, unsigned 
long *value, char *type,
>kallsyms = rcu_dereference_sched(mod->kallsyms);
>if (symnum < kallsyms->num_symtab) {
>*value = kallsyms->symtab[symnum].st_value;
> -  *type = kallsyms->symtab[symnum].st_info;
> +  *type = kallsyms->symtab[symnum].st_size;
>strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
>strlcpy(module_name, mod->name, MODULE_NAME_LEN);
>*exported = is_exported(name, *value, mod);

This is fine if st_size is really unused, but how sure are you of that?

grepping for st_size throws up some hits that appear ELF-related, but
I've not investigated them in detail.

(The fact that struct stat has an identically named field throws up
a load of false positives too.)


$ git describe --tags
v4.20-rc3-93-g92b419289cee

$ rg -m1 '[\.>]st_size'  --iglob '!**/tools/**' --iglob '!**/vdso*' --iglob 
'!**/scripts/**' --iglob '!**/usr/**' --iglob '!**/samples/**' | cat
| kernel/kexec_file.c:  if (sym->st_size != size) {

Symbols in kexec kernel.

| fs/stat.c:tmp.st_size = stat->size;
| Documentation/networking/tls.txt:  sendfile(sock, file, &offset, 
stat.st_size);
| net/9p/client.c:  ret->st_rdev, ret->st_size, ret->st_blksize,
| net/9p/protocol.c:&stbuf->st_rdev, 
&stbuf->st_size,
| fs/9p/vfs_inode_dotl.c:   i_size_write(inode, stat->st_size);
| fs/hostfs/hostfs_user.c:  p->size = buf->st_size;
| arch/powerpc/boot/mktree.c:   nblks = (st.st_size + IMGBLK) / IMGBLK;
| arch/alpha/kernel/osf_sys.c:  tmp.st_size = lstat->size;
| arch/x86/ia32/sys_ia32.c: __put_user(stat->size, &ubuf->st_size) ||

Not Elf_Sym.

| arch/x86/kernel/machine_kexec_64.c:sym->st_size);

Symbols in kexec kernel.

| arch/sparc/boot/piggyback.c:  st4(buffer + 12, s.st_size);
| arch/sparc/kernel/sys_sparc32.c:  err |= put_user(stat->size, 
&statbuf->st_size);
| arch/um/os-Linux/file.c:  .ust_size= src->st_size,/* 
total size, in bytes */
| arch/um/os-Linux/start_up.c:  size = (buf.st_size + UM_KERN_PAGE_SIZE) & 
~(UM_KERN_PAGE_SIZE - 1);
| arch/s390/kernel/compat_linux.c:  tmp.st_size = stat->size;
| arch/arm/kernel/sys_oabi-compat.c:tmp.st_size = stat->size;
| arch/mips/boot/compressed/calc_vmlinuz_load_addr.c:   vmlinux_size = 
(uint64_t)sb.st_size;
| drivers/net/ethernet/marvell/sky2.c:  hw->st_idx = RING_NEXT(hw->st_idx, 
hw->st_size);

Not Elf_Sym.


[ added Miroslav to CC, just in case he would like to check :) ]

I have just double checked as well, and am fairly certain that the
Elf_Sym st_size field is not used to apply module relocations in any
arches, and it is not used in the core module loader nor in the module
kallsyms code. We'd like to avoid overwriting st_info in any case, to
fix kallsyms on Thumb-2 and also so that livepatch won't run into any
issues with delayed relocations, should livepatch support ever expand
to arches (e.g., arm) that rely on st_info for module relocations.

Thanks,

Jessica

Re: [PATCH v2] module: make it clearer when we're handling kallsyms symbols vs exported symbols

[Jessica Yu]

+++ Miroslav Benes [29/11/18 09:18 +0100]:

On Fri, 23 Nov 2018, Jessica Yu wrote:


The module loader internally works with both exported symbols
represented as struct kernel_symbol, as well as Elf symbols from a
module's symbol table. It's hard to distinguish sometimes which type of
symbol we're handling given that some helper function names are not
consistent or helpful. Take get_ksymbol() for instance - are we
looking for an exported symbol or a kallsyms symbol here? Or symname()
and kernel_symbol_name() - which function handles an exported symbol and
which one an Elf symbol?

Clean up and unify the function naming scheme a bit to make it clear
which kind of symbol we're handling. This change only affects static
functions internal to the module loader.

Signed-off-by: Jessica Yu 


Reviewed-by: Miroslav Benes 


Applied, thanks!

Jessica

Re: [PATCH tip/core/rcu 24/41] modules: Replace synchronize_sched() and call_rcu_sched()

[Jessica Yu]

+++ Paul E. McKenney [11/11/18 11:43 -0800]:

Now that synchronize_rcu() waits for preempt-disable regions of code
as well as RCU read-side critical sections, synchronize_sched() can
be replaced by synchronize_rcu().  Similarly, call_rcu_sched() can be
replaced by call_rcu().  This commit therefore makes these changes.

Signed-off-by: Paul E. McKenney 
Cc: Jessica Yu 


Acked-by: Jessica Yu 

Thanks!


---
kernel/module.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..99b46c32d579 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2159,7 +2159,7 @@ static void free_module(struct module *mod)
/* Remove this module from bug list, this uses list_del_rcu */
module_bug_cleanup(mod);
/* Wait for RCU-sched synchronizing before releasing mod->list and 
buglist. */
-   synchronize_sched();
+   synchronize_rcu();
mutex_unlock(&module_mutex);

/* This may be empty, but that's OK */
@@ -3507,15 +3507,15 @@ static noinline int do_init_module(struct module *mod)
/*
 * We want to free module_init, but be aware that kallsyms may be
 * walking this with preempt disabled.  In all the failure paths, we
-* call synchronize_sched(), but we don't want to slow down the success
+* call synchronize_rcu(), but we don't want to slow down the success
 * path, so use actual RCU here.
 * Note that module_alloc() on most architectures creates W+X page
 * mappings which won't be cleaned up until do_free_init() runs.  Any
 * code such as mark_rodata_ro() which depends on those mappings to
 * be cleaned up needs to sync with the queued work - ie
-* rcu_barrier_sched()
+* rcu_barrier()
 */
-   call_rcu_sched(&freeinit->rcu, do_free_init);
+   call_rcu(&freeinit->rcu, do_free_init);
mutex_unlock(&module_mutex);
wake_up_all(&module_wq);

@@ -3526,7 +3526,7 @@ static noinline int do_init_module(struct module *mod)
fail:
/* Try to protect us from buggy refcounters. */
mod->state = MODULE_STATE_GOING;
-   synchronize_sched();
+   synchronize_rcu();
module_put(mod);
blocking_notifier_call_chain(&module_notify_list,
 MODULE_STATE_GOING, mod);
@@ -3819,7 +3819,7 @@ static int load_module(struct load_info *info, const char 
__user *uargs,
 ddebug_cleanup:
ftrace_release_mod(mod);
dynamic_debug_remove(mod, info->debug);
-   synchronize_sched();
+   synchronize_rcu();
kfree(mod->args);
 free_arch_cleanup:
module_arch_cleanup(mod);
@@ -3834,7 +3834,7 @@ static int load_module(struct load_info *info, const char 
__user *uargs,
mod_tree_remove(mod);
wake_up_all(&module_wq);
/* Wait for RCU-sched synchronizing before releasing mod->list. */
-   synchronize_sched();
+   synchronize_rcu();
mutex_unlock(&module_mutex);
 free_module:
/* Free lock-classes; relies on the preceding sync_rcu() */
--
2.17.1

Re: [PATCH v3] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Dave Martin [13/11/18 13:57 +]:

On Tue, Nov 13, 2018 at 12:27:45PM +0100, Vincent Whitchurch wrote:

Thumb-2 functions have the lowest bit set in the symbol value in the
symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
generated from the output of nm, and nm clears the lowest bit.

 $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
  95947: 8015dc89   686 FUNCGLOBAL DEFAULT2 show_interrupts
 $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
 8015dc88 T show_interrupts
 $ cat /proc/kallsyms | grep show_interrupts
 8015dc88 T show_interrupts

However, for modules, the kallsyms uses the values in the symbol table
without modification, so for functions in modules, the lowest bit is set
in kallsyms.

 $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
268: 00e144 FUNCGLOBAL DEFAULT2 tun_get_socket
 $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
 00e0 T tun_get_socket
 $ cat /proc/kallsyms | grep tun_get_socket
 7fcd30e1 t tun_get_socket  [tun]

Because of this, the offset of the crashing instruction shown in oopses
is incorrect when the crash is in a module.  For example, given a
tun_get_socket which starts like this,

 00e0 :
   e0:   b500push{lr}
   e2:   f7ff fffe   bl  0 <__gnu_mcount_nc>
   e6:   4b08ldr r3, [pc, #32]
   e8:   6942ldr r2, [r0, #20]
   ea:   429acmp r2, r3
   ec:   d002beq.n   f4 

a crash when tun_get_socket is called with NULL results in:

 PC is at tun_get_socket+0x7/0x2c [tun]
 pc : [<7fcdb0e8>]

which can result in the incorrect line being reported by gdb if this
symbol+offset is used there.  If the crash is on the first instruction
of a function, the "PC is at" line would also report the symbol name of
the preceding function.

To solve this, introduce a weak module_kallsyms_symbol_value() function
which arches can override to fix up these symbol values, and implement


(Jumping into this patch without having reviewed previous versions in
detail, so I may have misunderstood some things...)


Anyway:

I prefer this to the previous approach of directly hacking the symbol
values in the module kallsyms table.


this for Thumb-2.  We need to move elf_type() to st_other so that the
original value of st_info is preserved.


Are you sure modifying st_other won't break anything?

It's hard to imagine how ELF symbol visibility would be relevant in the
kernel, but some arches put other stuff in st_other.  See alpha,
powerpc, sh for example.  I haven't attempted to understand whether any
of those uses matters here.


Yeah, the st_other field is used to apply relocations in those arches
you mention. Overwriting st_info/st_other only "works" here in the
sense that that field is no longer needed after applying relocations
(add_kallsyms() is called in post_relocation() in the module loader).
I agree it's hacky to reuse the field in that way, though.


Ideally, we wouldn't abuse st_info to store elf_type() in the first
place, but that may be messier to solve.  kallsyms could wrap the
ElfXX_Sym in another struct to store extra metadata for example, but it
would increase runtime memory consumption.


Yeah, I've always thought that was ugly. I think it was done as a
small optimization for kallsyms, so that we're not looking up what
char to print for every symbol, so the st_info field was repurposed
for this.


Another option would be wedge STT_FUNC in bit 7 of st_info, say.
Since elf_type() always yields an ASCII char, that bit shouldn't be
used for anything today.  We would of course need to wrap further
access to st_info to mask that bit off as appropriate.


Hm, actually on second thought, I don't think the st_size field is
used *anywhere* in the module loader, not in the arch-specific
relocation code nor in kallsyms. Perhaps that field could be used to
store the output of elf_type instead. Then we won't run into any
issues with delayed relocations with livepatch, as the st_size field
isn't used at all for applying relocs anyway. Since this field is not
used at runtime, I think we can use st_size instead of st_other or
st_info, which are two fields that some arches do use to apply relocs.
Would be great if someone could confirm/fact-check me on this.

Thanks!

Jessica

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [09/11/18 14:53 +0100]:

On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:

+++ Vincent Whitchurch [01/11/18 16:29 +0100]:
> On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> > Could this be done in modpost? I'm guessing the answer is no as some
> > relocations may rely on that bit being set in st_value, right?
> > Therefore we can only clear the bit _after_ relocations to the module
> > are applied at runtime, correct?
>
> Yes, that's correct, it needs to be done after the relocations.
>
> > I'm not against having an arch-specific kallsyms fixup function, my
> > only concern would be if this would interfere with the delayed
> > relocations livepatching does, if there are plans in the future to
> > have livepatching on arm (IIRC there was an attempt at a port in
> > 2016). If there exists some Thumb-2 relocation types that rely on that
> > lowest bit in st_value being set in order to be applied, and we clear
> > it permanently from the symtab, then livepatching wouldn't be able to
> > apply those types of relocations anymore. If this is a legitimate
> > concern, then perhaps an alternative solution would be to have an
> > arch-specific kallsyms symbol-value-fixup function for accesses to
> > sym.st_value, without modifying the module symtab.
>
> I'm not familiar with livepatching, but yes, if it needs to do
> relocations later I guess we should preserve the original value.

Yeah, I think the symtab needs to remain unmodified for livepatch to
be able to do delayed relocations after module load.

> I gave the alternative solution a go and it seems to work.
> add_kallsyms() currently overwrites st_info so I had to move the
> elf_type to the unused st_other field instead to preserve st_info:

I think that's fine, I think the only usages of st_other I've found
are during relocations, and the field is big enough for a char, so it
should be fine to reuse it afterwards.


If livepatch can do relocations later, doesn't that mean that we _can't_
reuse st_other for storing the elf_type?  OTOH, the current code
overwrites st_info, and I see that used from various archs' relocation
functions too, so perhaps I'm missing something.


D'oh, I just contradicted myself. Yes, you are correct. AFAIK the only
reason we haven't run into issues yet in livepatch is because the only
arches it currently supports (x86, s390, powerpc) don't depend on
st_info for module relocations. But yes, that's an outstanding problem
that will need to be fixed if livepatch gains support on more arches,
we shouldn't be overwriting that field. And after grepping around I do
see that st_other is used in powerpc, alpha, sh for module
relocations. I suggested in my other reply to your v3 patch that
reusing st_size instead of st_info might work, since it's not used in
the module loader/kallsyms, and arches don't seem to use it for module
relocs. Maybe that would work?

Jessica

Re: [PATCH v5 2/2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [04/12/18 15:14 +0100]:

Thumb-2 functions have the lowest bit set in the symbol value in the
symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
generated from the output of nm, and nm clears the lowest bit.

$ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
 95947: 8015dc89   686 FUNCGLOBAL DEFAULT2 show_interrupts
$ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
8015dc88 T show_interrupts
$ cat /proc/kallsyms | grep show_interrupts
8015dc88 T show_interrupts

However, for modules, the kallsyms uses the values in the symbol table
without modification, so for functions in modules, the lowest bit is set
in kallsyms.

$ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
   333: 2d4d36 FUNCGLOBAL DEFAULT1 tun_get_socket
$ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
2d4c T tun_get_socket
$ cat /proc/kallsyms | grep tun_get_socket
7f802d4d t tun_get_socket  [tun]

Because of this, the symbol+offset of the crashing instruction shown in
oopses is incorrect when the crash is in a module.  For example, given a
tun_get_socket which starts like this,

2d4c :
2d4c:   6943ldr r3, [r0, #20]
2d4e:   4a07ldr r2, [pc, #28]
2d50:   4293cmp r3, r2

a crash when tun_get_socket is called with NULL results in:

PC is at tun_xdp+0xa3/0xa4 [tun]
pc : [<7f802d4c>]

As can be seen, the "PC is at" line reports the wrong symbol name, and
the symbol+offset will point to the wrong source line if it is passed to
gdb.

To solve this, add a way for archs to fixup the reading of these module
kallsyms values, and use that to clear the lowest bit for function
symbols on Thumb-2.

After the fix:

# cat /proc/kallsyms | grep tun_get_socket
7f802d4c t tun_get_socket   [tun]

PC is at tun_get_socket+0x0/0x24 [tun]
pc : [<7f802d4c>]

Signed-off-by: Vincent Whitchurch 


Looks good, could I get an ACK from an ARM maintainer? (Russell?)

Also, do you mind if I drop the module_ prefix from
module_kallsyms_symbol_value()? I recently submitted some internal
module kallsyms cleanups [1] and there we have the newly renamed
kallsyms_symbol_name(), so I think it'd be nice if we kept the naming
scheme consistent with just kallsyms_symbol_value(). I could just do
this myself if you don't want to respin a v6.

Thanks!

Jessica

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git/commit/?h=modules-next&id=2d25bc55235314d869459c574be14e8faa73aca3


---
v5: Use/move local variables to reduce calls and keep lines short.  Use const 
arg.
v4: Split out st_value overwrite change.  Add HAVE* macro to avoid function 
call.
v3: Do not overwrite st_value
v2: Fix build warning with !MODULES

arch/arm/include/asm/module.h | 11 +++
include/linux/module.h|  7 +++
kernel/module.c   | 45 +++
3 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 9e81b7c498d8..c7bcf0347801 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -61,4 +61,15 @@ u32 get_module_plt(struct module *mod, unsigned long loc, 
Elf32_Addr val);
MODULE_ARCH_VERMAGIC_ARMTHUMB \
MODULE_ARCH_VERMAGIC_P2V

+#ifdef CONFIG_THUMB2_KERNEL
+#define HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE
+static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym)
+{
+   if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+   return sym->st_value & ~1;
+
+   return sym->st_value;
+}
+#endif
+
#endif /* _ASM_ARM_MODULE_H */
diff --git a/include/linux/module.h b/include/linux/module.h
index fce6b4335e36..12146257eb5d 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -486,6 +486,13 @@ struct module {
#define MODULE_ARCH_INIT {}
#endif

+#ifndef HAVE_ARCH_MODULE_KALLSYMS_SYMBOL_VALUE
+static inline unsigned long module_kallsyms_symbol_value(const Elf_Sym *sym)
+{
+   return sym->st_value;
+}
+#endif
+
extern struct mutex module_mutex;

/* FIXME: It'd be nice to isolate modules during init, too, so they
diff --git a/kernel/module.c b/kernel/module.c
index 3d86a38b580c..9364017fdc21 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3922,7 +3922,7 @@ static const char *get_ksymbol(struct module *mod,
   unsigned long *offset)
{
unsigned int i, best = 0;
-   unsigned long nextval;
+   unsigned long nextval, bestval;
struct mod_kallsyms *kallsyms = rcu_dereference_sched(mod->kallsyms);

/* At worse, next value is at end of module */
@@ -3931,10 +3931,15 @@ static const char *get_ksymbol(struct module *mod,
else
nextval = (unsigned 
long)mod->core_layout.base+mod->core_layout.text_size;

+   bestval = module_kallsyms_symbol_value(&kallsyms->symtab[best]);
+
/* Scan for clos

[PATCH] modsign: log module name in the event of an error

[Jessica Yu]

Now that we have the load_info struct all initialized (including
info->name, which contains the name of the module) before
module_sig_check(), make the load_info struct and hence module name
available to mod_verify_sig() so that we can log the module name in the
event of an error.

Signed-off-by: Jessica Yu 
---
 kernel/module-internal.h | 26 +-
 kernel/module.c  | 22 +-
 kernel/module_signing.c  |  9 ++---
 3 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 915e123a430f..ddeb1241455c 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -9,4 +9,28 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
-extern int mod_verify_sig(const void *mod, unsigned long *_modlen);
+#include 
+#include 
+
+struct load_info {
+   const char *name;
+   /* pointer to module in temporary copy, freed at end of load_module() */
+   struct module *mod;
+   Elf_Ehdr *hdr;
+   unsigned long len;
+   Elf_Shdr *sechdrs;
+   char *secstrings, *strtab;
+   unsigned long symoffs, stroffs;
+   struct _ddebug *debug;
+   unsigned int num_debug;
+   bool sig_ok;
+#ifdef CONFIG_KALLSYMS
+   unsigned long mod_kallsyms_init_off;
+#endif
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
+};
+
+extern int mod_verify_sig(const void *mod, struct load_info *info,
+ unsigned long *_modlen);
diff --git a/kernel/module.c b/kernel/module.c
index ba45a84e4287..8bdd7e255274 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -307,26 +307,6 @@ int unregister_module_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
-struct load_info {
-   const char *name;
-   /* pointer to module in temporary copy, freed at end of load_module() */
-   struct module *mod;
-   Elf_Ehdr *hdr;
-   unsigned long len;
-   Elf_Shdr *sechdrs;
-   char *secstrings, *strtab;
-   unsigned long symoffs, stroffs;
-   struct _ddebug *debug;
-   unsigned int num_debug;
-   bool sig_ok;
-#ifdef CONFIG_KALLSYMS
-   unsigned long mod_kallsyms_init_off;
-#endif
-   struct {
-   unsigned int sym, str, mod, vers, info, pcpu;
-   } index;
-};
-
 /*
  * We require a truly strong try_module_get(): 0 means success.
  * Otherwise an error is returned due to ongoing or failed
@@ -2778,7 +2758,7 @@ static int module_sig_check(struct load_info *info, int 
flags)
memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) 
== 0) {
/* We truncate the module to discard the signature */
info->len -= markerlen;
-   err = mod_verify_sig(mod, &info->len);
+   err = mod_verify_sig(mod, info, &info->len);
}
 
if (!err) {
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844bee4a..caeea810242d 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -45,7 +45,8 @@ struct module_signature {
 /*
  * Verify the signature on a module.
  */
-int mod_verify_sig(const void *mod, unsigned long *_modlen)
+int mod_verify_sig(const void *mod, struct load_info *info,
+  unsigned long *_modlen)
 {
struct module_signature ms;
size_t modlen = *_modlen, sig_len;
@@ -65,7 +66,8 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
*_modlen = modlen;
 
if (ms.id_type != PKEY_ID_PKCS7) {
-   pr_err("Module is not signed with expected PKCS#7 message\n");
+   pr_err("%s: Module is not signed with expected PKCS#7 
message\n",
+  info->name);
return -ENOPKG;
}
 
@@ -76,7 +78,8 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
ms.__pad[0] != 0 ||
ms.__pad[1] != 0 ||
ms.__pad[2] != 0) {
-   pr_err("PKCS#7 signature info has unexpected non-zero 
params\n");
+   pr_err("%s: PKCS#7 signature info has unexpected non-zero 
params\n",
+  info->name);
return -EBADMSG;
}
 
-- 
2.16.4

Re: [PATCH] modsign: log module name in the event of an error

[Jessica Yu]

+++ Joe Perches [29/06/18 21:04 -0700]:

On Fri, 2018-06-29 at 17:53 +0200, Jessica Yu wrote:

Now that we have the load_info struct all initialized (including
info->name, which contains the name of the module) before
module_sig_check(), make the load_info struct and hence module name
available to mod_verify_sig() so that we can log the module name in the
event of an error.

[]

diff --git a/kernel/module-internal.h b/kernel/module-internal.h

[]

+extern int mod_verify_sig(const void *mod, struct load_info *info,
+ unsigned long *_modlen);
diff --git a/kernel/module.c b/kernel/module.c

[]

@@ -2778,7 +2758,7 @@ static int module_sig_check(struct load_info *info, int 
flags)
memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) 
== 0) {
/* We truncate the module to discard the signature */
info->len -= markerlen;
-   err = mod_verify_sig(mod, &info->len);
+   err = mod_verify_sig(mod, info, &info->len);


This is the only place this is used correct?
So why pass info and info->member?

info should be enough


Ah yeah you're right, thanks!

Jessica

[PATCH v2] modsign: log module name in the event of an error

[Jessica Yu]

Now that we have the load_info struct all initialized (including
info->name, which contains the name of the module) before
module_sig_check(), make the load_info struct and hence module name
available to mod_verify_sig() so that we can log the module name in the
event of an error.

Signed-off-by: Jessica Yu 
---
v2:
 - remove _modlen argument and just use the passed info struct

 kernel/module-internal.h | 25 -
 kernel/module.c  | 22 +-
 kernel/module_signing.c  | 12 +++-
 3 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 915e123a430f..79c9be2dbbe9 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -9,4 +9,27 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
-extern int mod_verify_sig(const void *mod, unsigned long *_modlen);
+#include 
+#include 
+
+struct load_info {
+   const char *name;
+   /* pointer to module in temporary copy, freed at end of load_module() */
+   struct module *mod;
+   Elf_Ehdr *hdr;
+   unsigned long len;
+   Elf_Shdr *sechdrs;
+   char *secstrings, *strtab;
+   unsigned long symoffs, stroffs;
+   struct _ddebug *debug;
+   unsigned int num_debug;
+   bool sig_ok;
+#ifdef CONFIG_KALLSYMS
+   unsigned long mod_kallsyms_init_off;
+#endif
+   struct {
+   unsigned int sym, str, mod, vers, info, pcpu;
+   } index;
+};
+
+extern int mod_verify_sig(const void *mod, struct load_info *info);
diff --git a/kernel/module.c b/kernel/module.c
index ba45a84e4287..8a45986fd728 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -307,26 +307,6 @@ int unregister_module_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_module_notifier);
 
-struct load_info {
-   const char *name;
-   /* pointer to module in temporary copy, freed at end of load_module() */
-   struct module *mod;
-   Elf_Ehdr *hdr;
-   unsigned long len;
-   Elf_Shdr *sechdrs;
-   char *secstrings, *strtab;
-   unsigned long symoffs, stroffs;
-   struct _ddebug *debug;
-   unsigned int num_debug;
-   bool sig_ok;
-#ifdef CONFIG_KALLSYMS
-   unsigned long mod_kallsyms_init_off;
-#endif
-   struct {
-   unsigned int sym, str, mod, vers, info, pcpu;
-   } index;
-};
-
 /*
  * We require a truly strong try_module_get(): 0 means success.
  * Otherwise an error is returned due to ongoing or failed
@@ -2778,7 +2758,7 @@ static int module_sig_check(struct load_info *info, int 
flags)
memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) 
== 0) {
/* We truncate the module to discard the signature */
info->len -= markerlen;
-   err = mod_verify_sig(mod, &info->len);
+   err = mod_verify_sig(mod, info);
}
 
if (!err) {
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 937c844bee4a..f2075ce8e4b3 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -45,10 +45,10 @@ struct module_signature {
 /*
  * Verify the signature on a module.
  */
-int mod_verify_sig(const void *mod, unsigned long *_modlen)
+int mod_verify_sig(const void *mod, struct load_info *info)
 {
struct module_signature ms;
-   size_t modlen = *_modlen, sig_len;
+   size_t sig_len, modlen = info->len;
 
pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
@@ -62,10 +62,11 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
if (sig_len >= modlen)
return -EBADMSG;
modlen -= sig_len;
-   *_modlen = modlen;
+   info->len = modlen;
 
if (ms.id_type != PKEY_ID_PKCS7) {
-   pr_err("Module is not signed with expected PKCS#7 message\n");
+   pr_err("%s: Module is not signed with expected PKCS#7 
message\n",
+  info->name);
return -ENOPKG;
}
 
@@ -76,7 +77,8 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
ms.__pad[0] != 0 ||
ms.__pad[1] != 0 ||
ms.__pad[2] != 0) {
-   pr_err("PKCS#7 signature info has unexpected non-zero 
params\n");
+   pr_err("%s: PKCS#7 signature info has unexpected non-zero 
params\n",
+  info->name);
return -EBADMSG;
}
 
-- 
2.16.4

Re: [PATCH v2 8/8] jump_table: move entries into ro_after_init region

[Jessica Yu]

+++ Ard Biesheuvel [02/07/18 20:11 +0200]:

The __jump_table sections emitted into the core kernel and into
each module consist of statically initialized references into
other parts of the code, and with the exception of entries that
point into init code, which are defused at post-init time, these
data structures are never modified.

So let's move them into the ro_after_init section, to prevent them
from being corrupted inadvertently by buggy code, or deliberately
by an attacker.

Signed-off-by: Ard Biesheuvel 


For module parts:

Acked-by: Jessica Yu 


---
arch/arm/kernel/vmlinux-xip.lds.S |  1 +
arch/s390/kernel/vmlinux.lds.S|  1 +
include/asm-generic/vmlinux.lds.h | 11 +++
kernel/module.c   |  9 +
4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/vmlinux-xip.lds.S 
b/arch/arm/kernel/vmlinux-xip.lds.S
index 3593d5c1acd2..763c41068ecc 100644
--- a/arch/arm/kernel/vmlinux-xip.lds.S
+++ b/arch/arm/kernel/vmlinux-xip.lds.S
@@ -118,6 +118,7 @@ SECTIONS
RW_DATA_SECTION(L1_CACHE_BYTES, PAGE_SIZE, THREAD_SIZE)
.data.ro_after_init : AT(ADDR(.data.ro_after_init) - LOAD_OFFSET) {
*(.data..ro_after_init)
+   JUMP_TABLE_DATA
}
_edata = .;

diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index f0414f52817b..a7cf61e46f88 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -67,6 +67,7 @@ SECTIONS
__start_ro_after_init = .;
.data..ro_after_init : {
 *(.data..ro_after_init)
+   JUMP_TABLE_DATA
}
EXCEPTION_TABLE(16)
. = ALIGN(PAGE_SIZE);
diff --git a/include/asm-generic/vmlinux.lds.h 
b/include/asm-generic/vmlinux.lds.h
index e373e2e10f6a..ed6befa4c47b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -256,10 +256,6 @@
STRUCT_ALIGN(); \
*(__tracepoints)\
/* implement dynamic printk debug */\
-   . = ALIGN(8);   \
-   __start___jump_table = .;   \
-   KEEP(*(__jump_table))   \
-   __stop___jump_table = .;\
. = ALIGN(8);   \
__start___verbose = .;  \
KEEP(*(__verbose))  \
@@ -303,6 +299,12 @@
. = __start_init_task + THREAD_SIZE;\
__end_init_task = .;

+#define JUMP_TABLE_DATA
\
+   . = ALIGN(8);   \
+   __start___jump_table = .;   \
+   KEEP(*(__jump_table))   \
+   __stop___jump_table = .;
+
/*
 * Allow architectures to handle ro_after_init data on their
 * own by defining an empty RO_AFTER_INIT_DATA.
@@ -311,6 +313,7 @@
#define RO_AFTER_INIT_DATA  \
__start_ro_after_init = .;  \
*(.data..ro_after_init) \
+   JUMP_TABLE_DATA \
__end_ro_after_init = .;
#endif

diff --git a/kernel/module.c b/kernel/module.c
index 7cb82e0fcac0..0d4e320e41cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3349,6 +3349,15 @@ static struct module *layout_and_allocate(struct 
load_info *info, int flags)
 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
 */
ndx = find_sec(info, ".data..ro_after_init");
+   if (ndx)
+   info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
+   /*
+* Mark the __jump_table section as ro_after_init as well: these data
+* structures are never modified, with the exception of entries that
+* refer to code in the __init section, which are annotated as such
+* at module load time.
+*/
+   ndx = find_sec(info, "__jump_table");
if (ndx)
info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;

--
2.17.1

Re: [PATCH] ARM: module: fix modsign build error

[Jessica Yu]

+++ Russell King - ARM Linux [06/07/18 14:00 +0100]:

On Fri, Jul 06, 2018 at 02:48:47PM +0200, Arnd Bergmann wrote:

The asm/module.h header file can not be included standalone, which
breaks the module signing code after a recent change:

In file included from kernel/module-internal.h:13,
 from kernel/module_signing.c:17:
arch/arm/include/asm/module.h:37:27: error: 'struct module' declared inside 
parameter list will not be visible outside of this definition or declaration 
[-Werror]
 u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val);

This adds a forward declaration of struct module to make it all work.

Fixes: f314dfea16a0 ("modsign: log module name in the event of an error")
Signed-off-by: Arnd Bergmann 


We used to have a general rule that where an asm/foo.h header and
linux/foo.h header exists, and the linux/foo.h includes the asm/foo.h,
then the linux/foo.h header will be used for including in .c files
rather than the asm/ version of the same.  Is there a reason why
that can't be done here?


Generally yes, it's just that in this case module_signing.c neither
needs nor includes linux/module.h. The needed load_info struct
definition just requires definitions for Elf_[SPE]hdr, Elf_Addr,
Elf_Sym, etc. and those are all defined in asm/module.h.


That said, adding this is no bad thing.

Acked-by: Russell King 

Thanks.


Thanks! Queued in modules-next.

Jessica

Re: [PATCH 01/18] kernel: Use pr_fmt

[Jessica Yu]

+++ Joe Perches [10/05/18 08:45 -0700]:

Sometime in the future, it would be useful to convert pr_fmt from a
default simple define to use a default prefix with KBUILD_MODNAME.

There are files in kernel/ that use pr_, some with an embedded
prefix, that also do not have a specific pr_fmt define.

Add pr_fmt for those files.

There are some differences in output as some messages are now prefixed
with their KBUILD_MODNAME.

Miscellanea:

o Align multiline statements to open parenthesis
o Wrap and realign arguments to 80 columns where sensible
o Coalesce formats

Signed-off-by: Joe Perches 


Acked-by: Jessica Yu  (for module.c)


---
kernel/acct.c  |  2 ++
kernel/async.c | 14 ++--
kernel/audit_tree.c|  2 +-
kernel/backtracetest.c |  8 +++
kernel/crash_core.c| 29 ++---
kernel/exit.c  |  2 ++
kernel/hung_task.c | 13 +--
kernel/kprobes.c   | 20 ++---
kernel/module.c| 59 +++---
kernel/panic.c |  3 +++
kernel/params.c| 13 ++-
kernel/pid.c   |  2 ++
kernel/profile.c   |  2 ++
kernel/range.c |  2 +-
kernel/relay.c |  5 -
kernel/seccomp.c   |  4 +++-
kernel/signal.c| 10 +
kernel/smpboot.c   |  5 -
kernel/taskstats.c |  4 +++-
kernel/torture.c   |  6 +++--
kernel/tracepoint.c|  3 +++
kernel/workqueue.c |  2 ++
22 files changed, 122 insertions(+), 88 deletions(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index addf7732fb56..c3d393655f11 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -44,6 +44,8 @@
 * a struct file opened for write. Fixed. 2/6/2000, AV.
 */

+#define pr_fmt(fmt) fmt
+
#include 
#include 
#include 
diff --git a/kernel/async.c b/kernel/async.c
index a893d6170944..9a6ab6016713 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -120,8 +120,8 @@ static void async_run_entry_fn(struct work_struct *work)
/* 1) run (and print duration) */
if (initcall_debug && system_state < SYSTEM_RUNNING) {
pr_debug("calling  %lli_%pF @ %i\n",
-   (long long)entry->cookie,
-   entry->func, task_pid_nr(current));
+(long long)entry->cookie,
+entry->func, task_pid_nr(current));
calltime = ktime_get();
}
entry->func(entry->data, entry->cookie);
@@ -129,9 +129,9 @@ static void async_run_entry_fn(struct work_struct *work)
rettime = ktime_get();
delta = ktime_sub(rettime, calltime);
pr_debug("initcall %lli_%pF returned 0 after %lld usecs\n",
-   (long long)entry->cookie,
-   entry->func,
-   (long long)ktime_to_ns(delta) >> 10);
+(long long)entry->cookie,
+entry->func,
+(long long)ktime_to_ns(delta) >> 10);
}

/* 2) remove self from the pending queues */
@@ -300,8 +300,8 @@ void async_synchronize_cookie_domain(async_cookie_t cookie, 
struct async_domain
delta = ktime_sub(endtime, starttime);

pr_debug("async_continuing @ %i after %lli usec\n",
-   task_pid_nr(current),
-   (long long)ktime_to_ns(delta) >> 10);
+task_pid_nr(current),
+(long long)ktime_to_ns(delta) >> 10);
}
}
EXPORT_SYMBOL_GPL(async_synchronize_cookie_domain);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 67e6956c0b61..f34f90b4a346 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -739,7 +739,7 @@ static int audit_launch_prune(void)
prune_thread = kthread_run(prune_tree_thread, NULL,
"audit_prune_tree");
if (IS_ERR(prune_thread)) {
-   pr_err("cannot start thread audit_prune_tree");
+   pr_err("cannot start thread audit_prune_tree\n");
prune_thread = NULL;
return -ENOMEM;
}
diff --git a/kernel/backtracetest.c b/kernel/backtracetest.c
index 1323360d90e3..d10cc39b0134 100644
--- a/kernel/backtracetest.c
+++ b/kernel/backtracetest.c
@@ -19,7 +19,7 @@

static void backtrace_test_normal(void)
{
-   pr_info("Testing a backtrace from process context.\n");
+   pr_info("Testing a backtrace from process context\n");
pr_info("The following trace is a kernel self test and not a bug!\n");

dump_stack();
@@ -37,7 +37,7 @@ static DECLARE_TASKLET(backtrace_tasklet, 
&backtrace_test_irq_callback, 0);

static void backtrace_test_irq(void)
{
-   pr_info("Testing a backtrace from irq context.\n");
+   pr_info("Testing a backtr

[PATCH 0/3] lockdown/module: make module name available for module_sig_check()

[Jessica Yu]

Hi David,

The changes here involve cleaning up load_module() (patches 1 and 2) in
preparation for patch 3. The general idea is to do some preliminary module
section parsing and set up load info convenience variables earlier so that
we could log the module name during the module signature verification check
if it fails. Right now the module name is not logged if signature
verification fails, and it would be helpful to know which module failed
loading.

Currently, all patches are based on the lockdown tree:


http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=lockdown

But my plan is probably to take patches 1 and 2 through the modules-next
tree as they are generic cleanups, but I wanted to give you a heads up for
patch 3, which should probably be taken through the lockdown tree.

Thanks!

Jessica

---
Jessica Yu (3):
  module: make it clear when we're handling the module copy in info->hdr
  module: setup load info before module_sig_check()
  modsign: print module name along with error message

 kernel/module.c | 105 ++--
 1 file changed, 57 insertions(+), 48 deletions(-)

-- 
2.16.3

[PATCH 2/3] module: setup load info before module_sig_check()

[Jessica Yu]

We want to be able to log the module name in early error messages, such as
when module signature verification fails.  Previously, the module name is
set in layout_and_allocate(), meaning that any error messages that happen
before (such as those in module_sig_check()) won't be logged with a module
name, which isn't terribly helpful.

In order to do this, reshuffle the order in load_module() and set up
load info earlier so that we can log the module name along with these
error messages. This requires splitting rewrite_section_headers() out of
setup_load_info().

While we're at it, clean up and split up the operations done in
layout_and_allocate(), setup_load_info(), and rewrite_section_headers()
more cleanly so these functions only perform what their names suggest.

Signed-off-by: Jessica Yu 
---
 kernel/module.c | 77 -
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index e8eba00bfed7..ae824a6f1a03 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2491,7 +2491,11 @@ static char *get_modinfo(struct load_info *info, const 
char *tag)
Elf_Shdr *infosec = &info->sechdrs[info->index.info];
unsigned long size = infosec->sh_size;
 
-   for (p = (char *)infosec->sh_addr; p; p = next_string(p, &size)) {
+   /*
+* get_modinfo() calls made before rewrite_section_headers()
+* must use sh_offset, as sh_addr isn't set!
+*/
+   for (p = (char *)info->hdr + infosec->sh_offset; p; p = next_string(p, 
&size)) {
if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
return p + taglen + 1;
}
@@ -2960,17 +2964,7 @@ static int rewrite_section_headers(struct load_info 
*info, int flags)
}
 
/* Track but don't keep modinfo and version sections. */
-   if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
-   info->index.vers = 0; /* Pretend no __versions section! */
-   else
-   info->index.vers = find_sec(info, "__versions");
info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
-
-   info->index.info = find_sec(info, ".modinfo");
-   if (!info->index.info)
-   info->name = "(missing .modinfo section)";
-   else
-   info->name = get_modinfo(info, "name");
info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
return 0;
@@ -2987,16 +2981,18 @@ static int rewrite_section_headers(struct load_info 
*info, int flags)
 static int setup_load_info(struct load_info *info, int flags)
 {
unsigned int i;
-   int err;
 
/* Set up the convenience variables */
info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
info->secstrings = (void *)info->hdr
+ info->sechdrs[info->hdr->e_shstrndx].sh_offset;
 
-   err = rewrite_section_headers(info, flags);
-   if (err)
-   return err;
+   /* Try to find a name early so we can log errors with a module name */
+   info->index.info = find_sec(info, ".modinfo");
+   if (!info->index.info)
+   info->name = "(missing .modinfo section)";
+   else
+   info->name = get_modinfo(info, "name");
 
/* Find internal symbols and strings. */
for (i = 1; i < info->hdr->e_shnum; i++) {
@@ -3009,6 +3005,11 @@ static int setup_load_info(struct load_info *info, int 
flags)
}
}
 
+   if (info->index.sym == 0) {
+   pr_warn("%s: module has no symbols (stripped?)\n", info->name);
+   return -ENOEXEC;
+   }
+
info->index.mod = find_sec(info, ".gnu.linkonce.this_module");
if (!info->index.mod) {
pr_warn("%s: No module found in object\n",
@@ -3016,26 +3017,22 @@ static int setup_load_info(struct load_info *info, int 
flags)
return -ENOEXEC;
}
/* This is temporary: point mod into copy of data. */
-   info->mod = (void *)info->sechdrs[info->index.mod].sh_addr;
+   info->mod = (void *)info->hdr + 
info->sechdrs[info->index.mod].sh_offset;
 
/*
-* If we didn't load the .modinfo 'name' field, fall back to
+* If we didn't load the .modinfo 'name' field earlier, fall back to
 * on-disk struct mod 'name' field.
 */
if (!info->name)
info->name = info->mod->name;
 
-   if (info->index.sym == 0) {
-   pr_warn("%s: module has no symbols (stripped?)\n", info->name);
-   return -ENOEXEC;
-   }
+   

[PATCH 1/3] module: make it clear when we're handling the module copy in info->hdr

[Jessica Yu]

In load_module(), it's not always clear whether we're handling the
temporary module copy in info->hdr (which is freed at the end of
load_module()) or if we're handling the module already allocated and
copied to it's final place. Adding an info->mod field and using it
whenever we're handling the temporary copy makes that explicitly clear.

Signed-off-by: Jessica Yu 
---
 kernel/module.c | 42 +-
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 9c1709a05037..e8eba00bfed7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -312,6 +312,8 @@ EXPORT_SYMBOL(unregister_module_notifier);
 
 struct load_info {
const char *name;
+   /* pointer to module in temporary copy, freed at end of load_module() */
+   struct module *mod;
Elf_Ehdr *hdr;
unsigned long len;
Elf_Shdr *sechdrs;
@@ -2979,14 +2981,13 @@ static int rewrite_section_headers(struct load_info 
*info, int flags)
  * search for module section index etc), and do some basic section
  * verification.
  *
- * Return the temporary module pointer (we'll replace it with the final
- * one when we move the module sections around).
+ * Set info->mod to the temporary copy of the module in info->hdr. The final 
one
+ * will be allocated in move_module().
  */
-static struct module *setup_load_info(struct load_info *info, int flags)
+static int setup_load_info(struct load_info *info, int flags)
 {
unsigned int i;
int err;
-   struct module *mod;
 
/* Set up the convenience variables */
info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
@@ -2995,7 +2996,7 @@ static struct module *setup_load_info(struct load_info 
*info, int flags)
 
err = rewrite_section_headers(info, flags);
if (err)
-   return ERR_PTR(err);
+   return err;
 
/* Find internal symbols and strings. */
for (i = 1; i < info->hdr->e_shnum; i++) {
@@ -3012,30 +3013,30 @@ static struct module *setup_load_info(struct load_info 
*info, int flags)
if (!info->index.mod) {
pr_warn("%s: No module found in object\n",
info->name ?: "(missing .modinfo name field)");
-   return ERR_PTR(-ENOEXEC);
+   return -ENOEXEC;
}
/* This is temporary: point mod into copy of data. */
-   mod = (void *)info->sechdrs[info->index.mod].sh_addr;
+   info->mod = (void *)info->sechdrs[info->index.mod].sh_addr;
 
/*
 * If we didn't load the .modinfo 'name' field, fall back to
 * on-disk struct mod 'name' field.
 */
if (!info->name)
-   info->name = mod->name;
+   info->name = info->mod->name;
 
if (info->index.sym == 0) {
pr_warn("%s: module has no symbols (stripped?)\n", info->name);
-   return ERR_PTR(-ENOEXEC);
+   return -ENOEXEC;
}
 
info->index.pcpu = find_pcpusec(info);
 
/* Check module struct version now, before we try to use module. */
-   if (!check_modstruct_version(info, mod))
-   return ERR_PTR(-ENOEXEC);
+   if (!check_modstruct_version(info, info->mod))
+   return -ENOEXEC;
 
-   return mod;
+   return 0;
 }
 
 static int check_modinfo(struct module *mod, struct load_info *info, int flags)
@@ -3330,25 +3331,24 @@ core_param(module_blacklist, module_blacklist, charp, 
0400);
 
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
-   /* Module within temporary copy. */
struct module *mod;
unsigned int ndx;
int err;
 
-   mod = setup_load_info(info, flags);
-   if (IS_ERR(mod))
-   return mod;
+   err = setup_load_info(info, flags);
+   if (err)
+   return ERR_PTR(err);
 
if (blacklisted(info->name))
return ERR_PTR(-EPERM);
 
-   err = check_modinfo(mod, info, flags);
+   err = check_modinfo(info->mod, info, flags);
if (err)
return ERR_PTR(err);
 
/* Allow arches to frob section contents and sizes.  */
err = module_frob_arch_sections(info->hdr, info->sechdrs,
-   info->secstrings, mod);
+   info->secstrings, info->mod);
if (err < 0)
return ERR_PTR(err);
 
@@ -3367,11 +3367,11 @@ static struct module *layout_and_allocate(struct 
load_info *info, int flags)
/* Determine total sizes, and put offsets in sh_entsize.  For now
   this is done generically; there doesn't appear to be any
   special cases for the architectures. */
-   layout_sections(mod, inf

[PATCH 3/3] modsign: print module name along with error message

[Jessica Yu]

It is useful to know which module failed signature verification, so
print the module name along with the error message.

Signed-off-by: Jessica Yu 
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index ae824a6f1a03..8670585eb5f7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2807,7 +2807,7 @@ static int module_sig_check(struct load_info *info, int 
flags,
reason = "Loading of module with unavailable key";
decide:
if (sig_enforce) {
-   pr_notice("%s is rejected\n", reason);
+   pr_notice("%s: %s is rejected\n", info->name, reason);
return -EKEYREJECTED;
}
 
-- 
2.16.3

Re: [PATCH v2] module: Implement sig_unenforce parameter

[Jessica Yu]

+++ Brett T. Warden [06/06/18 12:44 -0700]:

When CONFIG_MODULE_SIG_FORCE is enabled, also provide a boot-time-only
parameter, module.sig_unenforce, to disable signature enforcement. This
allows distributions to ship with signature verification enforcement
enabled by default, but for users to elect to disable it without
recompiling, to support building and loading out-of-tree modules.

Signed-off-by: Brett T. Warden 
---

Added CONFIG_X86 guards around use of boot_params since this structure
is arch-specific.

.../admin-guide/kernel-parameters.txt |  4 +++
kernel/module.c   | 31 +--
2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1beb30d8d7fc..87909e021558 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2380,6 +2380,10 @@
Note that if CONFIG_MODULE_SIG_FORCE is set, that
is always true, so this option does nothing.

+   module.sig_unenforce
+   [KNL] This parameter allows modules without signatures
+   to be loaded, overriding CONFIG_MODULE_SIG_FORCE.
+
module_blacklist=  [KNL] Do not load a comma-separated list of
modules.  Useful for debugging problem modules.

diff --git a/kernel/module.c b/kernel/module.c
index c9bea7f2b43e..27f23d85e1ba 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -64,6 +64,7 @@
#include 
#include 
#include 
+#include 
#include 
#include "module-internal.h"

@@ -274,9 +275,13 @@ static void module_assert_mutex_or_preempt(void)
}

static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
-#ifndef CONFIG_MODULE_SIG_FORCE
+#ifdef CONFIG_MODULE_SIG_FORCE
+/* Allow disabling module signature requirement by adding boot param */
+static bool sig_unenforce;
+module_param(sig_unenforce, bool_enable_only, 0444);
+#else /* !CONFIG_MODULE_SIG_FORCE */
module_param(sig_enforce, bool_enable_only, 0644);
-#endif /* !CONFIG_MODULE_SIG_FORCE */
+#endif /* CONFIG_MODULE_SIG_FORCE */

/*
 * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
@@ -415,6 +420,10 @@ extern const s32 __start___kcrctab_unused[];
extern const s32 __start___kcrctab_unused_gpl[];
#endif

+#ifdef CONFIG_X86
+extern struct boot_params boot_params;
+#endif
+
#ifndef CONFIG_MODVERSIONS
#define symversion(base, idx) NULL
#else
@@ -4243,6 +4252,24 @@ static const struct file_operations 
proc_modules_operations = {
static int __init proc_modules_init(void)
{
proc_create("modules", 0, NULL, &proc_modules_operations);
+
+#ifdef CONFIG_MODULE_SIG_FORCE
+#ifdef CONFIG_X86
+   switch (boot_params.secure_boot) {
+   case efi_secureboot_mode_unset:
+   case efi_secureboot_mode_unknown:
+   case efi_secureboot_mode_disabled:
+   /*
+* sig_unenforce is only applied if SecureBoot is not
+* enabled.
+*/
+   sig_enforce = !sig_unenforce;
+   }
+#else
+   sig_enforce = !sig_unenforce;
+#endif
+#endif


I don't want to rope in Secure Boot specifics and arch-specific code
(other than what's in arch/) into the module loader. I also don't want
to change the current semantics of CONFIG_MODULE_SIG_FORCE, for any
existing users who just want to set a build-time policy and already
assume that unsigned modules can't be loaded, period. Lastly, having
both sig_enforce and sig_unenforce parameters is really confusing.
Might as well just make it one parameter - sig_enforce - and make that
toggleable under a certain configuration.

It sounds like you want to ship with signature enforcement enabled by
default, but stil want to make this toggleable for users. So maybe
something in between CONFIG_MODULE_SIG and CONFIG_MODULE_SIG_FORCE.
Half baked suggestion: Would a new config option that enforces module
signatures by default but makes sig_enforce toggleable for users suit
your use case? This would be less strict than CONFIG_MODULE_SIG_FORCE.

Thanks,

Jessica

[GIT PULL] modules for v4.18

[Jessica Yu]

Hi Linus, 


The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:

 Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)

are available in the git repository at:

 ssh://g...@gitolite.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git 
tags/modules-for-v4.18

for you to fetch changes up to c554b89868015d86cd330d9cc10656c3756352a5:

 module: Allow to always show the status of modsign (2018-04-16 23:49:33 +0200)


Modules updates for v4.18

Summary of modules changes for the 4.18 merge window:

- Minor code cleanup and also allow sig_enforce param to be shown in
 sysfs with CONFIG_MODULE_SIG_FORCE

Signed-off-by: Jessica Yu 


Jia Zhang (2):
 module: Do not access sig_enforce directly
 module: Allow to always show the status of modsign

kernel/module.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

[GIT PULL] Modules updates for v4.19

[Jessica Yu]

Linus,

Please pull below to receive modules updates for the 4.19 merge window.
Details can be found in the signed tag.

Thanks,

Jessica

---
The following changes since commit ce397d215ccd07b8ae3f71db689aedb85d56ab40:

 Linux 4.18-rc1 (2018-06-17 08:04:49 +0900)

are available in the Git repository at:

 ssh://g...@gitolite.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git 
tags/modules-for-v4.19

for you to fetch changes up to 9be936f4b3a2ec101f54cff9cf1a6abf67263c50:

 kernel/module: Use kmemdup to replace kmalloc+memcpy (2018-08-02 18:03:17 
+0200)


Modules updates for v4.19

Summary of modules changes for the 4.19 merge window:

- Fix modules kallsyms for livepatch. Livepatch modules can have
 SHN_UNDEF symbols in their module symbol tables for later symbol
 resolution, but kallsyms shouldn't be returning these symbols

- Some code cleanups and minor reshuffling in load_module() were done to
 log the module name when module signature verification fails

Signed-off-by: Jessica Yu 


Arnd Bergmann (1):
 ARM: module: fix modsign build error

Jason A. Donenfeld (1):
 module: print sensible error code

Jessica Yu (4):
 module: exclude SHN_UNDEF symbols from kallsyms api
 module: make it clear when we're handling the module copy in info->hdr
 module: setup load info before module_sig_check()
 modsign: log module name in the event of an error

Masahiro Yamada (1):
 module: replace VMLINUX_SYMBOL_STR() with __stringify() or string literal

zhong jiang (1):
 kernel/module: Use kmemdup to replace kmalloc+memcpy

arch/arm/include/asm/module.h |   1 +
include/linux/module.h|   4 +-
kernel/module-internal.h  |  25 +++-
kernel/module.c   | 143 +++---
kernel/module_signing.c   |  12 ++--
5 files changed, 100 insertions(+), 85 deletions(-)

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

[Jessica Yu]

+++ Nadav Amit [04/12/18 17:34 -0800]:

When module memory is about to be freed, there is no apparent reason to
make it (and its data) executable, but that's exactly what is done
today. This is not efficient and not secure.

There are various theories why it was done, but none of them seem as
something that really require it today. nios2 uses kmalloc for module
memory, but anyhow it does not change the PTEs of the module memory.  In
x86, changing vmalloc'd memory mappings also modifies the direct mapping
alias, but the NX-bit is not modified in such way.

So let's remove it. Andy suggested that the changes of the PTEs can be
avoided (excluding the direct-mapping alias), which is true. However,
in x86 it requires some cleanup of the contiguous page allocator, which
is outside of the scope of this patch-set.

Cc: Rick P Edgecombe 
Cc: Will Deacon 
Cc: Andy Lutomirski 
Signed-off-by: Nadav Amit 


[ Thanks Andrea Parri for the cc ]

Regarding the patch subject, don't you mean "Do not make module
memory executable" or "Do not unset nx" instead of "Do not set nx"?
Hm, these double negatives are confusing :-)

I think this also needs to be done in the load_module() error path.
See the bug_cleanup label. There, module_disable_{ro,nx}() are called
before module deallocation.

I am not sure why all this was made executable before freeing in the
first place.  Tried to dig through the commit history and the first
commit that introduced this behavior was 448694a1d50 ("module: undo
module RONX protection correctly"). There, the behavior was changed
from W+NX to W+X before releasing the module. But AFAIK from the
changelog, there was no real technical reason behind it, it stemmed
out of the complaint of code asymmetry :-/

Jessica


---
kernel/module.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 7cb207249437..57c5b23746e7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void)
mutex_unlock(&module_mutex);
}

-static void disable_ro_nx(const struct module_layout *layout)
+static void module_restore_mappings(const struct module_layout *layout)
{
-   if (rodata_enabled) {
-   frob_text(layout, set_memory_rw);
-   frob_rodata(layout, set_memory_rw);
-   frob_ro_after_init(layout, set_memory_rw);
-   }
-   frob_rodata(layout, set_memory_x);
-   frob_ro_after_init(layout, set_memory_x);
-   frob_writable_data(layout, set_memory_x);
+   /*
+* First, make the mappings of the code non-executable to prevent
+* transient W+X mappings from being set when the text is set as RW.
+*/
+   frob_text(layout, set_memory_nx);
+
+   if (!rodata_enabled)
+   return;
+
+   /*
+* Second, set the memory as writable. Although the module memory is
+* about to be freed, these calls are required (at least on x86) to
+* restore the direct map to its "correct" state.
+*/
+   frob_text(layout, set_memory_rw);
+   frob_rodata(layout, set_memory_rw);
+   frob_ro_after_init(layout, set_memory_rw);
}

#else
-static void disable_ro_nx(const struct module_layout *layout) { }
+static void module_restore_mappings(const struct module_layout *layout) { }
static void module_enable_nx(const struct module *mod) { }
static void module_disable_nx(const struct module *mod) { }
#endif
@@ -2173,7 +2182,7 @@ static void free_module(struct module *mod)
mutex_unlock(&module_mutex);

/* This may be empty, but that's OK */
-   disable_ro_nx(&mod->init_layout);
+   module_restore_mappings(&mod->init_layout);
module_arch_freeing_init(mod);
module_memfree(mod->init_layout.base);
kfree(mod->args);
@@ -2183,7 +2192,7 @@ static void free_module(struct module *mod)
lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);

/* Finally, free the core (containing the module structure) */
-   disable_ro_nx(&mod->core_layout);
+   module_restore_mappings(&mod->core_layout);
module_memfree(mod->core_layout.base);
}

@@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod)
#endif
module_enable_ro(mod, true);
mod_tree_remove_init(mod);
-   disable_ro_nx(&mod->init_layout);
+   module_restore_mappings(&mod->init_layout);
module_arch_freeing_init(mod);
mod->init_layout.base = NULL;
mod->init_layout.size = 0;
--
2.17.1

Re: [PATCH v6 modules-next 1/2] module: Overwrite st_size instead of st_info

[Jessica Yu]

+++ Vincent Whitchurch [14/12/18 17:05 +0100]:

st_info is currently overwritten after relocation and used to store the
elf_type().  However, we're going to need it fix kallsyms on ARM's
Thumb-2 kernels, so preserve st_info and overwrite the st_size field
instead.  st_size is neither used by the module core nor by any
architecture.

Reviewed-by: Miroslav Benes 
Reviewed-by: Dave Martin 
Signed-off-by: Vincent Whitchurch 


Applied both. (Thanks for rebasing!)

Thanks,

Jessica


---
v6: Add Miroslav Benes' Reviewed-by
v5: Add Dave Martin's Reviewed-by
v4: Split out to separate patch.  Use st_size instead of st_other.
v1-v3: See PATCH 2/2

kernel/module.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 1b5edf78694c..b36ff8a3d562 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2684,7 +2684,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)

/* Set types up while we still have access to sections. */
for (i = 0; i < mod->kallsyms->num_symtab; i++)
-   mod->kallsyms->symtab[i].st_info
+   mod->kallsyms->symtab[i].st_size
= elf_type(&mod->kallsyms->symtab[i], info);

/* Now populate the cut down core kallsyms for after init. */
@@ -4070,7 +4070,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long 
*value, char *type,
kallsyms = rcu_dereference_sched(mod->kallsyms);
if (symnum < kallsyms->num_symtab) {
*value = kallsyms->symtab[symnum].st_value;
-   *type = kallsyms->symtab[symnum].st_info;
+   *type = kallsyms->symtab[symnum].st_size;
strlcpy(name, kallsyms_symbol_name(kallsyms, symnum), 
KSYM_NAME_LEN);
strlcpy(module_name, mod->name, MODULE_NAME_LEN);
*exported = is_exported(name, *value, mod);
--
2.11.0

Re: [PATCH v9 RESEND 0/4] KASLR feature to randomize each loadable module

[Jessica Yu]

+++ Edgecombe, Rick P [12/12/18 23:05 +]:

On Wed, 2018-11-28 at 01:40 +, Edgecombe, Rick P wrote:

On Tue, 2018-11-27 at 11:21 +0100, Daniel Borkmann wrote:
> On 11/27/2018 01:19 AM, Edgecombe, Rick P wrote:
> > On Mon, 2018-11-26 at 16:36 +0100, Jessica Yu wrote:
> > > +++ Rick Edgecombe [20/11/18 15:23 -0800]:
> >
> > [snip]
> > > Hi Rick!
> > >
> > > Sorry for the delay. I'd like to take a step back and ask some broader
> > > questions -
> > >
> > > - Is the end goal of this patchset to randomize loading kernel modules,
> > > or
> > > most/all
> > >executable kernel memory allocations, including bpf, kprobes, etc?
> >
> > Thanks for taking a look!
> >
> > It started with the goal of just randomizing modules (hence the name), but
> > I
> > think there is maybe value in randomizing the placement of all runtime
> > added
> > executable code. Beyond just trying to make executable code placement less
> > deterministic in general, today all of the usages have the property of
> > starting
> > with RW permissions and then becoming RO executable, so there is the
> > benefit
> > of
> > narrowing the chances a bug could successfully write to it during the RW
> > window.
> >
> > > - It seems that a lot of complexity and heuristics are introduced just
> > > to
> > >accommodate the potential fragmentation that can happen when the
> > > module
> > > vmalloc
> > >space starts to get fragmented with bpf filters. I'm partial to the
> > > idea of
> > >splitting or having bpf own its own vmalloc space, similar to what
> > > Ard
> > > is
> > > already
> > >implementing for arm64.
> > >
> > >So a question for the bpf and x86 folks, is having a dedicated
> > > vmalloc
> > > region
> > >(as well as a seperate bpf_alloc api) for bpf feasible or desirable
> > > on
> > > x86_64?
> >
> > I actually did some prototyping and testing on this. It seems there would
> > be
> > some slowdown from the required changes to the JITed code to support
> > calling
> > back from the vmalloc region into the kernel, and so module space would
> > still be
> > the preferred region.
>
> Yes, any runtime slow-down would be no-go as BPF sits in the middle of
> critical
> networking fast-path and e.g. on XDP or tc layer and is used in load-
> balancing,
> firewalling, DDoS protection scenarios, some recent examples in [0-3].
>
>   [0] http://vger.kernel.org/lpc-networking2018.html#session-10
>   [1] http://vger.kernel.org/lpc-networking2018.html#session-15
>   [2] https://blog.cloudflare.com/how-to-drop-10-million-packets/
>   [3] http://vger.kernel.org/lpc-bpf2018.html#session-1
>
> > >If bpf filters need to be within 2 GB of the core kernel, would it
> > > make
> > > sense
> > >to carve out a portion of the current module region for bpf
> > > filters?  According
> > >to Documentation/x86/x86_64/mm.txt, the module region is ~1.5 GB. I
> > > am
> > > doubtful
> > >that any real system will actually have 1.5 GB worth of kernel
> > > modules
> > > loaded.
> > >Is there a specific reason why that much space is dedicated to kernel
> > > modules,
> > >and would it be feasible to split that region cleanly with bpf?
> >
> > Hopefully someone from BPF side of things will chime in, but my
> > understanding
> > was that they would like even more space than today if possible and so
> > they
> > may
> > not like the reduced space.
>
> I wouldn't mind of the region is split as Jessica suggests but in a way
> where
> there would be _no_ runtime regressions for BPF. This might also allow to
> have
> more flexibility in sizing the area dedicated for BPF in future, and could
> potentially be done in similar way as Ard was proposing recently [4].
>
>   [4] https://patchwork.ozlabs.org/project/netdev/list/?series=9

CCing Ard.

The benefit of sharing the space, for randomization at least, is that you can
spread the allocations over a larger area.

I think there are also other benefits to unifying how this memory is managed
though, rather than spreading it further. Today there are various patterns and
techniques used like calling different combinations of set_memory_* before
freeing, zeroing in modules or setting invalid instructions like BPF does,
etc.
There is also special care to be taken on vfree-ing executable memory. So this
way things 

Re: [PATCH] vmlinux.lds.h: drop unused __vermagic

[Jessica Yu]

+++ Mathias Krause [30/12/18 13:40 +0100]:

The reference to '__vermagic' is a relict from v2.5 times. And even
there it had a very short life time, from v2.5.59 (commit 1d411b80ee18
("Module Sanity Check") in the historic tree) to v2.5.69 (commit
67ac5b866bda ("[PATCH] complete modinfo section")).

Neither current kernels nor modules contain a '__vermagic' section any
more, so get rid of it.

Signed-off-by: Mathias Krause 
Cc: Rusty Russell 
Cc: Jessica Yu 


Thanks for the cleanup.

Reviewed-by: Jessica Yu 


---
include/asm-generic/vmlinux.lds.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h 
b/include/asm-generic/vmlinux.lds.h
index 3d7a6a9c2370..530ce1e7a1ec 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -332,7 +332,6 @@
__start_rodata = .; \
*(.rodata) *(.rodata.*) \
RO_AFTER_INIT_DATA  /* Read only after init */  \
-   KEEP(*(__vermagic)) /* Kernel version magic */  \
. = ALIGN(8);   \
__start___tracepoints_ptrs = .; \
KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
--
2.19.2

[GIT PULL] Modules updates for v4.21

[Jessica Yu]

Linus,

Please pull below to receive modules updates for the 4.21 merge window.
Details can be found in the signed tag.

Thanks,

Jessica

---

The following changes since commit 651022382c7f8da46cb4872a545ee1da6d097d2a:

 Linux 4.20-rc1 (2018-11-04 15:37:52 -0800)

are available in the Git repository at:

 ssh://g...@gitolite.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git 
tags/modules-for-v4.21

for you to fetch changes up to 93d77e7f1410c366050d6035dcba1a5167c7cf0b:

 ARM: module: Fix function kallsyms on Thumb-2 (2018-12-14 20:27:29 +0100)


Modules updates for v4.21

Summary of modules changes for the 4.21 merge window:

- Some modules-related kallsyms cleanups and a kallsyms fix for ARM.

- Include keys from the secondary keyring in module signature
 verification.

Signed-off-by: Jessica Yu 


Jessica Yu (1):
 module: make it clearer when we're handling kallsyms symbols vs exported 
symbols

Ke Wu (1):
 modsign: use all trusted keys to verify module signature

Vincent Whitchurch (2):
 module: Overwrite st_size instead of st_info
 ARM: module: Fix function kallsyms on Thumb-2

arch/arm/include/asm/module.h |  11 
include/linux/module.h|   7 +++
kernel/module.c   | 123 --
kernel/module_signing.c   |   3 +-
4 files changed, 92 insertions(+), 52 deletions(-)

Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules

[Jessica Yu]

+++ Miroslav Benes [29/10/18 14:24 +0100]:



diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index dd23655fda3a..490e56070a7e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr,
#endif
}

+#ifdef CONFIG_LIVEPATCH
+   /*
+* For livepatching, switch to the saved section header info for .plt
+* stored in mod->klp_info. This is needed so that livepatch is able to
+* call apply_relocate_add() after patch module load.
+*/
+   if (is_livepatch_module(me))
+   me->arch.core.plt = me->klp_info->sechdrs + 
me->arch.core.plt_shndx;
+#endif


I missed it before, but the hunk should be under "#ifdef
CONFIG_ARM64_MODULE_PLTS" protection similarly to ftrace_trampoline just
above. me->arch.core.plt may not exist otherwise.


Gah! Yes you are right, will fix.

Thanks,

Jessica

Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules

[Jessica Yu]

+++ Will Deacon [29/10/18 15:28 +]:

Hi Jessica,

On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:

The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.

Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.

Signed-off-by: Jessica Yu 
---

v2:
 - fix missing free_module_elf() in error path
 - move copy_module_elf() and module_finalize() out of post_relocation()
   to make error handling more clear
 - add braces to if-else block in arm64 module_frob_arch_sections()

arch/arm64/include/asm/module.h |  1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c  | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr   *plt;
int plt_num_entries;
int plt_max_entries;
+   int plt_shndx;
};


Does this mean we can drop the plt pointer from this struct altogether, and
simply offset into the section headers when applying the relocations?


Hmm, if everyone is OK with dropping the plt pointer from struct
mod_plt_sec, then I think we can simplify this patch even further.

With the plt shndx saved, we can additionally pass a pointer to
sechdrs to module_emit_plt_entry(), and with that just offset into the
section headers as you suggest. Since livepatch *already* passes in
the correct copy of the section headers (mod->klp_info->sechdrs) to
apply_relocate_add(), we wouldn't even need to modify the arm64
module_finalize() to change mod->arch.core.plt to point into
mod->klp_info->sechdrs anymore and we can drop all the changes to the
module loader too.

Something like the following maybe?

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac10fa066487 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,7 +22,7 @@

#ifdef CONFIG_ARM64_MODULE_PLTS
struct mod_plt_sec {
-   struct elf64_shdr   *plt;
+   int plt_shndx;
int plt_num_entries;
int plt_max_entries;
};
@@ -37,10 +37,12 @@ struct mod_arch_specific {
};
#endif

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym);

-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+   void *loc, u64 val);

#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..3cd744a1cbc2 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -16,13 +16,15 @@ static bool in_init(const struct module *mod, void *loc)
return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
}

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym)
{
-   struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
- &mod->arch.init;
-   struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
-   int i = pltsec->plt_num_entries;
+   struct mod_plt_sec *plt_info = !in_init(mod, loc) ? &mod->arch.core :
+   &mod->arch.init;
+   Elf64_Shdr *pltsec = sechdrs 

Re: [PATCH] modsign: use all trusted keys to verify module signature

[Jessica Yu]

+++ Ke Wu [22/10/18 15:26 -0700]:

Make mod_verify_sig to use all trusted keys. This allows keys in
secondary_trusted_keys to be used to verify PKCS#7 signature on a
kernel module.

Signed-off-by: Ke Wu 


Thanks for the ping, I had missed this patch.

David, could I get an ACK please?

Thanks!

Jessica


---
kernel/module_signing.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f2075ce8e4b3..a8b923ba1a39 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -83,6 +83,6 @@ int mod_verify_sig(const void *mod, struct load_info *info)
}

return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
- NULL, VERIFYING_MODULE_SIGNATURE,
+ (void *)1UL, VERIFYING_MODULE_SIGNATURE,
  NULL, NULL);
}
--
2.19.1.568.g152ad8e336-goog

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [31/10/18 09:42 +0100]:

Thumb-2 functions have the lowest bit set in the symbol value in the
symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
generated from the output of nm, and nm clears the lowest bit.

$ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
 95947: 8015dc89   686 FUNCGLOBAL DEFAULT2 show_interrupts
$ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
8015dc88 T show_interrupts
$ cat /proc/kallsyms | grep show_interrupts
8015dc88 T show_interrupts

However, for modules, the kallsyms uses the values in the symbol table
without modification, so for functions in modules, the lowest bit is set
in kallsyms.

$ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
   268: 00e144 FUNCGLOBAL DEFAULT2 tun_get_socket
$ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
00e0 T tun_get_socket
$ cat /proc/kallsyms | grep tun_get_socket
7fcd30e1 t tun_get_socket  [tun]

Because of this, the offset of the crashing instruction shown in oopses
is incorrect when the crash is in a module.  For example, given a
tun_get_socket which starts like this,

00e0 :
  e0:   b500push{lr}
  e2:   f7ff fffe   bl  0 <__gnu_mcount_nc>
  e6:   4b08ldr r3, [pc, #32]
  e8:   6942ldr r2, [r0, #20]
  ea:   429acmp r2, r3
  ec:   d002beq.n   f4 

a crash when tun_get_socket is called with NULL results in:

PC is at tun_get_socket+0x7/0x2c [tun]
pc : [<7fcdb0e8>]

which can result in the incorrect line being reported by gdb if this
symbol+offset is used there.  If the crash is on the first instruction
of a function, the "PC is at" line would also report the symbol name of
the preceding function.

To solve this, fix up these symbols like nm does.  For this, we need a
new hook in the generic module loading code, before the symbols' st_info
is overwritten by add_kallsyms().  After the fix:

$ cat /proc/kallsyms | grep tun_get_socket
7fcd30e0 t tun_get_socket  [tun]

PC is at tun_get_socket+0x8/0x2c [tun]
pc : [<7fcdb0e8>]

Signed-off-by: Vincent Whitchurch 
---
v2: Fix build warning with !MODULES



Hi Vincent,

Just a few questions -

Could this be done in modpost? I'm guessing the answer is no as some
relocations may rely on that bit being set in st_value, right?
Therefore we can only clear the bit _after_ relocations to the module
are applied at runtime, correct?

I'm not against having an arch-specific kallsyms fixup function, my
only concern would be if this would interfere with the delayed
relocations livepatching does, if there are plans in the future to
have livepatching on arm (IIRC there was an attempt at a port in
2016). If there exists some Thumb-2 relocation types that rely on that
lowest bit in st_value being set in order to be applied, and we clear
it permanently from the symtab, then livepatching wouldn't be able to
apply those types of relocations anymore. If this is a legitimate
concern, then perhaps an alternative solution would be to have an
arch-specific kallsyms symbol-value-fixup function for accesses to
sym.st_value, without modifying the module symtab.


Thanks,

Jessica


arch/arm/kernel/module.c | 14 ++
include/linux/moduleloader.h |  3 +++
kernel/module.c  |  6 ++
3 files changed, 23 insertions(+)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..771f86318d84 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -399,6 +399,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr 
*sechdrs,
return 0;
}

+#if defined(CONFIG_THUMB2_KERNEL) && defined(CONFIG_KALLSYMS)
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
+{
+   int i;
+
+   for (i = 0; i < kallsyms->num_symtab; i++) {
+   Elf_Sym *sym = &kallsyms->symtab[i];
+
+   if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+   sym->st_value &= ~1;
+   }
+}
+#endif
+
void
module_arch_cleanup(struct module *mod)
{
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..b1ac5584eaa5 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
/* Any cleanup before freeing mod->module_init */
void module_arch_freeing_init(struct module *mod);

+struct mod_kallsyms;
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms);
+
#ifdef CONFIG_KASAN
#include 
#define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..ded4f4b49824 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2659,6 +2659,10 @@ static void layout_symtab(struct module *mod, struct 
load_info *info)
mod->init_layout.size = debug_align(mod->init_layout.size);
}

+void __weak module_fixup_kallsy

Re: [PATCH] modsign: use all trusted keys to verify module signature

[Jessica Yu]

+++ Ke Wu [22/10/18 15:26 -0700]:

Make mod_verify_sig to use all trusted keys. This allows keys in
secondary_trusted_keys to be used to verify PKCS#7 signature on a
kernel module.

Signed-off-by: Ke Wu 
---
kernel/module_signing.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f2075ce8e4b3..a8b923ba1a39 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -83,6 +83,6 @@ int mod_verify_sig(const void *mod, struct load_info *info)
}

return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
- NULL, VERIFYING_MODULE_SIGNATURE,
+ (void *)1UL, VERIFYING_MODULE_SIGNATURE,
  NULL, NULL);
}


I've just stumbled on commit 817aef260037f ("Replace magic for trusting
the secondary keyring with #define"), so we should probably use
VERIFY_USE_SECONDARY_KEYRING in place of (void *)1UL.

Thanks,

Jessica

Re: [PATCH v2] modsign: use all trusted keys to verify module signature

[Jessica Yu]

+++ Ke Wu [06/11/18 15:23 -0800]:

Thanks for the comment! I switched to use
VERIFY_USE_SECONDARY_KEYRING, please take a look.


Patch has been queued on modules-next. Thanks!

Jessica


On Tue, Nov 6, 2018 at 3:21 PM Ke Wu  wrote:


Make mod_verify_sig to use all trusted keys. This allows keys in
secondary_trusted_keys to be used to verify PKCS#7 signature on a
kernel module.

Signed-off-by: Ke Wu 
---
Changelog since v1:
- Use VERIFY_USE_SECONDARY_KEYRING rather than (void *)1UL

 kernel/module_signing.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f2075ce8e4b3..6b9a926fd86b 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -83,6 +83,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
}

return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
- NULL, VERIFYING_MODULE_SIGNATURE,
+ VERIFY_USE_SECONDARY_KEYRING,
+ VERIFYING_MODULE_SIGNATURE,
  NULL, NULL);
 }
--
2.19.1.930.g4563a0d9d0-goog




--
Ke Wu | Software Engineer | mik...@google.com | Google Inc.

Re: [PATCH v2 4/7] arm64/modules: Add rlimit checking for arm64 modules

[Jessica Yu]

+++ Dave Hansen [11/10/18 16:47 -0700]:

On 10/11/2018 04:31 PM, Rick Edgecombe wrote:

+   if (check_inc_mod_rlimit(size))
+   return NULL;
+
p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
module_alloc_base + MODULES_VSIZE,
gfp_mask, PAGE_KERNEL_EXEC, 0,
@@ -65,6 +68,8 @@ void *module_alloc(unsigned long size)
return NULL;
}

+   update_mod_rlimit(p, size);


Is there a reason we couldn't just rename all of the existing per-arch
module_alloc() calls to be called, say, "arch_module_alloc()", then put
this new rlimit code in a generic helper that does:


if (check_inc_mod_rlimit(size))
return NULL;

p = arch_module_alloc(...);

...

update_mod_rlimit(p, size);



I second this suggestion. Just make module_{alloc,memfree} generic,
non-weak functions that call the rlimit helpers in addition to the
arch-specific arch_module_{alloc,memfree} functions.

Jessica

Re: [PATCH for 4.19] tracepoint: Fix: out-of-bound tracepoint array iteration

[Jessica Yu]

+++ Mathieu Desnoyers [12/10/18 16:05 -0400]:

commit 46e0c9be206f ("kernel: tracepoints: add support for relative
references") changes the layout of the __tracepoint_ptrs section on
architectures supporting relative references. However, it does so
without turning struct tracepoint * const into const int * elsewhere in
the tracepoint code, which has the following side-effect:

tracepoint_module_{coming,going} invoke
tp_module_going_check_quiescent() with mod->tracepoints_ptrs
as first argument, and computes the end address of the array
for the second argument with:

 mod->tracepoints_ptrs + mod->num_tracepoints

However, because the type of mod->tracepoint_ptrs in module.h
has not been changed from pointer to int, it passes an end
pointer which is twice larger than the array, causing out-of-bound
array accesses.

Fix this by introducing a new typedef: tracepoint_ptr_t, which
is either "const int" on architectures that have PREL32 relocations,
or "struct tracepoint * const" on architectures that does not have
this feature.

Also provide a new tracepoint_ptr_defer() static inline to
encapsulate deferencing this type rather than duplicate code and
ugly idefs within the for_each_tracepoint_range() implementation.

This issue appears in 4.19-rc kernels, and should ideally be fixed
before the end of the rc cycle.

Signed-off-by: Mathieu Desnoyers 
Link: http://lkml.kernel.org/r/20180704083651.24360-7-ard.biesheu...@linaro.org
Cc: Michael Ellerman 
Cc: Ingo Molnar 
Cc: Steven Rostedt (VMware) 
Cc: Ard Biesheuvel 
Cc: Arnd Bergmann 
Cc: Benjamin Herrenschmidt 
Cc: Bjorn Helgaas 
Cc: Catalin Marinas 
Cc: James Morris 
Cc: James Morris 
Cc: Jessica Yu 
Cc: Josh Poimboeuf 
Cc: Kees Cook 
Cc: Nicolas Pitre 
Cc: Paul Mackerras 
Cc: Petr Mladek 
Cc: Russell King 
Cc: "Serge E. Hallyn" 
Cc: Sergey Senozhatsky 
Cc: Thomas Garnier 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Andrew Morton 
Cc: Linus Torvalds 
Cc: Greg Kroah-Hartman 


For module parts:

Acked-by: Jessica Yu 


---
include/linux/module.h  |  2 +-
include/linux/tracepoint-defs.h |  6 ++
include/linux/tracepoint.h  | 36 +
kernel/tracepoint.c | 24 --
4 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f807f15bebbe..cdab2451d6be 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -430,7 +430,7 @@ struct module {

#ifdef CONFIG_TRACEPOINTS
unsigned int num_tracepoints;
-   struct tracepoint * const *tracepoints_ptrs;
+   tracepoint_ptr_t *tracepoints_ptrs;
#endif
#ifdef HAVE_JUMP_LABEL
struct jump_entry *jump_entries;
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 22c5a46e9693..49ba9cde7e4b 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -35,6 +35,12 @@ struct tracepoint {
struct tracepoint_func __rcu *funcs;
};

+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+typedef const int tracepoint_ptr_t;
+#else
+typedef struct tracepoint * const tracepoint_ptr_t;
+#endif
+
struct bpf_raw_event_map {
struct tracepoint   *tp;
void*bpf_func;
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 041f7e56a289..538ba1a58f5b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -99,6 +99,29 @@ extern void syscall_unregfunc(void);
#define TRACE_DEFINE_ENUM(x)
#define TRACE_DEFINE_SIZEOF(x)

+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
+{
+   return offset_to_ptr(p);
+}
+
+#define __TRACEPOINT_ENTRY(name)   \
+   asm("  .section \"__tracepoints_ptrs\", \"a\"  \n"  
  \
+   "  .balign 4   \n"\
+   "  .long   __tracepoint_" #name " - .\n"\
+   "  .previous   \n")
+#else
+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
+{
+   return *p;
+}
+
+#define __TRACEPOINT_ENTRY(name)\
+   static tracepoint_ptr_t __tracepoint_ptr_##name __used   \
+   __attribute__((section("__tracepoints_ptrs"))) =   \
+   &__tracepoint_##name
+#endif
+
#endif /* _LINUX_TRACEPOINT_H */

/*
@@ -253,19 +276,6 @@ extern void syscall_unregfunc(void);
return static_key_false(&__tracepoint_##name.key);  \
}

-#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
-#define __TRACEPOINT_ENTRY(name)   \
-   asm("  .section \"__tracepoints_ptrs\", \"a\"  

Re: [PATCH for 4.19] tracepoint: Fix: tracepoint array element size mismatch

[Jessica Yu]

+++ Mathieu Desnoyers [13/10/18 15:10 -0400]:

commit 46e0c9be206f ("kernel: tracepoints: add support for relative
references") changes the layout of the __tracepoint_ptrs section on
architectures supporting relative references. However, it does so
without turning struct tracepoint * const into const int elsewhere in
the tracepoint code, which has the following side-effect:

Setting mod->num_tracepoints is done in by module.c:

   mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
sizeof(*mod->tracepoints_ptrs),
&mod->num_tracepoints);

Basically, since sizeof(*mod->tracepoints_ptrs) is a pointer size
(rather than sizeof(int)), num_tracepoints is erroneously set to half the
size it should be on 64-bit arch. So a module with an odd number of
tracepoints misses the last tracepoint due to effect of integer
division.

So in the module going notifier:

   for_each_tracepoint_range(mod->tracepoints_ptrs,
   mod->tracepoints_ptrs + mod->num_tracepoints,
   tp_module_going_check_quiescent, NULL);

the expression (mod->tracepoints_ptrs + mod->num_tracepoints) actually
evaluates to something within the bounds of the array, but miss the
last tracepoint if the number of tracepoints is odd on 64-bit arch.

Fix this by introducing a new typedef: tracepoint_ptr_t, which
is either "const int" on architectures that have PREL32 relocations,
or "struct tracepoint * const" on architectures that does not have
this feature.

Also provide a new tracepoint_ptr_defer() static inline to
encapsulate deferencing this type rather than duplicate code and
ugly idefs within the for_each_tracepoint_range() implementation.

This issue appears in 4.19-rc kernels, and should ideally be fixed
before the end of the rc cycle.

Signed-off-by: Mathieu Desnoyers 
Acked-by: Ard Biesheuvel 
Link: http://lkml.kernel.org/r/20180704083651.24360-7-ard.biesheu...@linaro.org
Cc: Michael Ellerman 
Cc: Ingo Molnar 
Cc: Steven Rostedt (VMware) 
Cc: Ard Biesheuvel 
Cc: Arnd Bergmann 
Cc: Benjamin Herrenschmidt 
Cc: Bjorn Helgaas 
Cc: Catalin Marinas 
Cc: James Morris 
Cc: James Morris 
Cc: Jessica Yu 
Cc: Josh Poimboeuf 
Cc: Kees Cook 
Cc: Nicolas Pitre 
Cc: Paul Mackerras 
Cc: Petr Mladek 
Cc: Russell King 
Cc: "Serge E. Hallyn" 
Cc: Sergey Senozhatsky 
Cc: Thomas Garnier 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Andrew Morton 
Cc: Linus Torvalds 
Cc: Greg Kroah-Hartman 


Whoops, acked the first version of the patch instead of this one.
In any case, for module parts:

Acked-by: Jessica Yu 


---
include/linux/module.h  |  3 ++-
include/linux/tracepoint-defs.h |  6 ++
include/linux/tracepoint.h  | 36 +
kernel/tracepoint.c | 24 --
4 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f807f15bebbe..e19ae08c7fb8 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -20,6 +20,7 @@
#include 
#include 
#include 
+#include 

#include 
#include 
@@ -430,7 +431,7 @@ struct module {

#ifdef CONFIG_TRACEPOINTS
unsigned int num_tracepoints;
-   struct tracepoint * const *tracepoints_ptrs;
+   tracepoint_ptr_t *tracepoints_ptrs;
#endif
#ifdef HAVE_JUMP_LABEL
struct jump_entry *jump_entries;
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 22c5a46e9693..49ba9cde7e4b 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -35,6 +35,12 @@ struct tracepoint {
struct tracepoint_func __rcu *funcs;
};

+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+typedef const int tracepoint_ptr_t;
+#else
+typedef struct tracepoint * const tracepoint_ptr_t;
+#endif
+
struct bpf_raw_event_map {
struct tracepoint   *tp;
void*bpf_func;
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 041f7e56a289..538ba1a58f5b 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -99,6 +99,29 @@ extern void syscall_unregfunc(void);
#define TRACE_DEFINE_ENUM(x)
#define TRACE_DEFINE_SIZEOF(x)

+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
+static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
+{
+   return offset_to_ptr(p);
+}
+
+#define __TRACEPOINT_ENTRY(name)   \
+   asm("  .section \"__tracepoints_ptrs\", \"a\"  \n"  
  \
+   "  .balign 4   \n"\
+   "  .long   __tracepoint_" #name " - .\n"\
+   "  .previous   \n")
+#else
+static inline struct tracepoint *tracepoint_ptr_deref(

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [01/11/18 16:29 +0100]:

On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:

Could this be done in modpost? I'm guessing the answer is no as some
relocations may rely on that bit being set in st_value, right?
Therefore we can only clear the bit _after_ relocations to the module
are applied at runtime, correct?


Yes, that's correct, it needs to be done after the relocations.


I'm not against having an arch-specific kallsyms fixup function, my
only concern would be if this would interfere with the delayed
relocations livepatching does, if there are plans in the future to
have livepatching on arm (IIRC there was an attempt at a port in
2016). If there exists some Thumb-2 relocation types that rely on that
lowest bit in st_value being set in order to be applied, and we clear
it permanently from the symtab, then livepatching wouldn't be able to
apply those types of relocations anymore. If this is a legitimate
concern, then perhaps an alternative solution would be to have an
arch-specific kallsyms symbol-value-fixup function for accesses to
sym.st_value, without modifying the module symtab.


I'm not familiar with livepatching, but yes, if it needs to do
relocations later I guess we should preserve the original value.


Yeah, I think the symtab needs to remain unmodified for livepatch to
be able to do delayed relocations after module load.


I gave the alternative solution a go and it seems to work.
add_kallsyms() currently overwrites st_info so I had to move the
elf_type to the unused st_other field instead to preserve st_info:


I think that's fine, I think the only usages of st_other I've found
are during relocations, and the field is big enough for a char, so it
should be fine to reuse it afterwards.


diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..f443d0ccd1a0 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -336,6 +336,16 @@ static const Elf_Shdr *find_mod_section(const Elf32_Ehdr 
*hdr,
extern void fixup_pv_table(const void *, unsigned long);
extern void fixup_smp(const void *, unsigned long);

+#ifdef CONFIG_THUMB2_KERNEL
+unsigned long symbol_value(Elf_Sym *sym)
+{
+   if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+   return sym->st_value & ~1;
+
+   return sym->st_value;
+}
+#endif
+
int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
struct module *mod)
{
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..6bf6118db37f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,8 @@ void module_arch_cleanup(struct module *mod);
/* Any cleanup before freeing mod->module_init */
void module_arch_freeing_init(struct module *mod);

+unsigned long symbol_value(Elf_Sym *sym);
+
#ifdef CONFIG_KASAN
#include 
#define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..871bf4450e9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct 
load_info *info)

/* Set types up while we still have access to sections. */
for (i = 0; i < mod->kallsyms->num_symtab; i++)
-   mod->kallsyms->symtab[i].st_info
+   mod->kallsyms->symtab[i].st_other
= elf_type(&mod->kallsyms->symtab[i], info);

/* Now populate the cut down core kallsyms for after init. */
@@ -3916,6 +3916,11 @@ static const char *symname(struct mod_kallsyms 
*kallsyms, unsigned int symnum)
return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
}

+unsigned long __weak symbol_value(Elf_Sym *sym)
+{
+   return sym->st_value;
+}
+
static const char *get_ksymbol(struct module *mod,
   unsigned long addr,
   unsigned long *size,
@@ -3934,6 +3939,9 @@ static const char *get_ksymbol(struct module *mod,
/* Scan for closest preceding symbol, and next symbol. (ELF
   starts real symbols at 1). */
for (i = 1; i < kallsyms->num_symtab; i++) {
+   unsigned long thisval = symbol_value(&kallsyms->symtab[i]);
+   unsigned long bestval = symbol_value(&kallsyms->symtab[best]);
+
if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
continue;

@@ -3943,21 +3951,21 @@ static const char *get_ksymbol(struct module *mod,
|| is_arm_mapping_symbol(symname(kallsyms, i)))
continue;

-   if (kallsyms->symtab[i].st_value <= addr
-   && kallsyms->symtab[i].st_value > 
kallsyms->symtab[best].st_value)
+   if (thisval <= addr
+   && thisval > bes

[PATCH] arm64/module: use plt section indices for relocations

[Jessica Yu]

Instead of saving a pointer to the .plt and .init.plt sections to apply
plt-based relocations, save and use their section indices instead.

The mod->arch.{core,init}.plt pointers were problematic for livepatch
because they pointed within temporary section headers (provided by the
module loader via info->sechdrs) that would be freed after module load.
Since livepatch modules may need to apply relocations post-module-load
(for example, to patch a module that is loaded later), using section
indices to offset into the section headers (instead of accessing them
through a saved pointer) allows livepatch modules on arm64 to pass in
their own copy of the section headers to apply_relocate_add() to apply
delayed relocations.

Signed-off-by: Jessica Yu 
---

Note: Addressed Will's comment about the pltsec -> plt_info rename and
removed that change to reduce unnecessary code churn. That is the only
change from the first version. I didn't include the Ack's for this reason
so let me know if this version is OK as well.

arch/arm64/include/asm/module.h |  8 +---
arch/arm64/kernel/module-plts.c | 36 
arch/arm64/kernel/module.c  |  9 +
3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 97d0ef12e2ff..f81c1847312d 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,7 +22,7 @@

#ifdef CONFIG_ARM64_MODULE_PLTS
struct mod_plt_sec {
-   struct elf64_shdr   *plt;
+   int plt_shndx;
int plt_num_entries;
int plt_max_entries;
};
@@ -36,10 +36,12 @@ struct mod_arch_specific {
};
#endif

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym);

-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+   void *loc, u64 val);

#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..c04b0d6abde0 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -16,12 +16,13 @@ static bool in_init(const struct module *mod, void *loc)
return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
}

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
  &mod->arch.init;
-   struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
+   struct plt_entry *plt = (struct plt_entry *)(sechdrs + 
pltsec->plt_shndx)->sh_addr;
int i = pltsec->plt_num_entries;
u64 val = sym->st_value + rela->r_addend;

@@ -43,11 +44,12 @@ u64 module_emit_plt_entry(struct module *mod, void *loc, 
const Elf64_Rela *rela,
}

#ifdef CONFIG_ARM64_ERRATUM_843419
-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+   void *loc, u64 val)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
  &mod->arch.init;
-   struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
+   struct plt_entry *plt = (struct plt_entry *)(sechdrs + 
pltsec->plt_shndx)->sh_addr;
int i = pltsec->plt_num_entries++;
u32 mov0, mov1, mov2, br;
int rd;
@@ -202,7 +204,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
unsigned long core_plts = 0;
unsigned long init_plts = 0;
Elf64_Sym *syms = NULL;
-   Elf_Shdr *tramp = NULL;
+   Elf_Shdr *pltsec, *tramp = NULL;
int i;

/*
@@ -211,9 +213,9 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
 */
for (i = 0; i < ehdr->e_shnum; i++) {
if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
-   mod->arch.core.plt = sechdrs + i;
+   mod->arch.core.plt_shndx = i;
else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
-   mod->arch.init.plt = sechdrs + i;
+   mod->arch.init.plt_sh

[PATCH v2] arm64/module: use plt section indices for relocations

[Jessica Yu]

Instead of saving a pointer to the .plt and .init.plt sections to apply
plt-based relocations, save and use their section indices instead.

The mod->arch.{core,init}.plt pointers were problematic for livepatch
because they pointed within temporary section headers (provided by the
module loader via info->sechdrs) that would be freed after module load.
Since livepatch modules may need to apply relocations post-module-load
(for example, to patch a module that is loaded later), using section
indices to offset into the section headers (instead of accessing them
through a saved pointer) allows livepatch modules on arm64 to pass in
their own copy of the section headers to apply_relocate_add() to apply
delayed relocations.

Signed-off-by: Jessica Yu 
---

v2: 


- Do sechdrs[pltsec->plt_shndx].sh_addr instead of pointer math

Note: Addressed Will's comment about the pltsec -> plt_info rename and
removed that change to reduce unnecessary code churn. I didn't include the
Ack's for this reason so let me know if this version is OK as well.

arch/arm64/include/asm/module.h |  8 +---
arch/arm64/kernel/module-plts.c | 36 
arch/arm64/kernel/module.c  |  9 +
3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 97d0ef12e2ff..f81c1847312d 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,7 +22,7 @@

#ifdef CONFIG_ARM64_MODULE_PLTS
struct mod_plt_sec {
-   struct elf64_shdr   *plt;
+   int plt_shndx;
int plt_num_entries;
int plt_max_entries;
};
@@ -36,10 +36,12 @@ struct mod_arch_specific {
};
#endif

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym);

-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+   void *loc, u64 val);

#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..a0efe30211d6 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -16,12 +16,13 @@ static bool in_init(const struct module *mod, void *loc)
return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
}

-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela 
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
  Elf64_Sym *sym)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
  &mod->arch.init;
-   struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
+   struct plt_entry *plt = (struct plt_entry 
*)sechdrs[pltsec->plt_shndx].sh_addr;
int i = pltsec->plt_num_entries;
u64 val = sym->st_value + rela->r_addend;

@@ -43,11 +44,12 @@ u64 module_emit_plt_entry(struct module *mod, void *loc, 
const Elf64_Rela *rela,
}

#ifdef CONFIG_ARM64_ERRATUM_843419
-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+   void *loc, u64 val)
{
struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
  &mod->arch.init;
-   struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
+   struct plt_entry *plt = (struct plt_entry 
*)sechdrs[pltsec->plt_shndx].sh_addr;
int i = pltsec->plt_num_entries++;
u32 mov0, mov1, mov2, br;
int rd;
@@ -202,7 +204,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
unsigned long core_plts = 0;
unsigned long init_plts = 0;
Elf64_Sym *syms = NULL;
-   Elf_Shdr *tramp = NULL;
+   Elf_Shdr *pltsec, *tramp = NULL;
int i;

/*
@@ -211,9 +213,9 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
 */
for (i = 0; i < ehdr->e_shnum; i++) {
if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
-   mod->arch.core.plt = sechdrs + i;
+   mod->arch.core.plt_shndx = i;
else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
-   mod->arch.init.plt = sechdrs + i;
+   mod->arch.init.plt_shndx

Re: [PATCH v2] arm64/module: use plt section indices for relocations

[Jessica Yu]

+++ Will Deacon [05/11/18 19:26 +]:

On Mon, Nov 05, 2018 at 07:53:23PM +0100, Jessica Yu wrote:

Instead of saving a pointer to the .plt and .init.plt sections to apply
plt-based relocations, save and use their section indices instead.

The mod->arch.{core,init}.plt pointers were problematic for livepatch
because they pointed within temporary section headers (provided by the
module loader via info->sechdrs) that would be freed after module load.
Since livepatch modules may need to apply relocations post-module-load
(for example, to patch a module that is loaded later), using section
indices to offset into the section headers (instead of accessing them
through a saved pointer) allows livepatch modules on arm64 to pass in
their own copy of the section headers to apply_relocate_add() to apply
delayed relocations.

Signed-off-by: Jessica Yu 
---

v2:

- Do sechdrs[pltsec->plt_shndx].sh_addr instead of pointer math

Note: Addressed Will's comment about the pltsec -> plt_info rename and
removed that change to reduce unnecessary code churn. I didn't include the
Ack's for this reason so let me know if this version is OK as well.


Thanks, Jessica!

Acked-by: Will Deacon 


arch/arm64/include/asm/module.h |  8 +---
arch/arm64/kernel/module-plts.c | 36 
arch/arm64/kernel/module.c  |  9 +
3 files changed, 30 insertions(+), 23 deletions(-)


Actually, I guess I should just queue this for 4.21 given that it's
completely self-contained.


Yes, that's fine :-) Then Torsten won't have to include this patch to his
patchset.

Thank you!

Jessica

[PATCH] arm64/module: use mod->klp_info section header information

[Jessica Yu]

The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.

Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.

Signed-off-by: Jessica Yu 
---

Hi!

This patch may be applied on top or merged with the 3rd patch. I
incoporated Miroslav's suggestions from the discussion. It's needed in
order for livepatch modules on arm64 to be able to call
apply_relocate_add() post-module-load, otherwise we could end up
accessing invalid pointers from apply_relocate_add().

arch/arm64/include/asm/module.h |  1 +
arch/arm64/kernel/module-plts.c | 10 --
arch/arm64/kernel/module.c  | 10 ++
kernel/module.c | 22 +-
4 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr   *plt;
int plt_num_entries;
int plt_max_entries;
+   int plt_shndx;
};

struct mod_arch_specific {
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..05067717dfc5 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -210,9 +210,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
 * entries. Record the symtab address as well.
 */
for (i = 0; i < ehdr->e_shnum; i++) {
-   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) {
mod->arch.core.plt = sechdrs + i;
-   else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
+   /*
+* Keep the section index for the .plt section for
+* livepatching. Note that .init.plt is irrelevant to
+* livepatch, so only the shndx for .plt is saved.
+*/
+   mod->arch.core.plt_shndx = i;
+   } else if (!strcmp(secstrings + sechdrs[i].sh_name, 
".init.plt"))
mod->arch.init.plt = sechdrs + i;
else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
 !strcmp(secstrings + sechdrs[i].sh_name,
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index dd23655fda3a..490e56070a7e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr,
#endif
}

+#ifdef CONFIG_LIVEPATCH
+   /*
+* For livepatching, switch to the saved section header info for .plt
+* stored in mod->klp_info. This is needed so that livepatch is able to
+* call apply_relocate_add() after patch module load.
+*/
+   if (is_livepatch_module(me))
+   me->arch.core.plt = me->klp_info->sechdrs + 
me->arch.core.plt_shndx;
+#endif
+
return 0;
}
diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..f3ac04cc9fc3 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr,

static int post_relocation(struct module *mod, const struct load_info *info)
{
+   int err;
+
/* Sort exception table now relocations are done. */
sort_extable(mod->extable, mod->extable + mod->num_exentries);

@@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const 
struct load_info *info)
/* Setup kallsyms-specific fields. */
add_kallsyms(mod, info);

+   if (is_livepatch_module(mod)) {
+   err = copy_module_elf(mod, info);
+   if (err < 0)
+   return err;
+   }
+
/* Arch-specific module finalizing. */
-   return module_finalize(info->hdr, info->sechdrs, mod);
+   err = module_final

Re: [PATCH RFC v3 0/3] Rlimit for module space

[Jessica Yu]

+++ Ard Biesheuvel [23/10/18 08:54 -0300]:

On 22 October 2018 at 20:06, Edgecombe, Rick P
 wrote:

On Sat, 2018-10-20 at 19:20 +0200, Ard Biesheuvel wrote:

Hi Rick,

On 19 October 2018 at 22:47, Rick Edgecombe 
wrote:
> If BPF JIT is on, there is no effective limit to prevent filling the entire
> module space with JITed e/BPF filters.

Why do BPF filters use the module space, and does this reason apply to
all architectures?

On arm64, we already support loading plain modules far away from the
core kernel (i.e. out of range for ordinary relative jump/calll
instructions), and so I'd expect BPF to be able to deal with this
already as well. So for arm64, I wonder why an ordinary vmalloc_exec()
wouldn't be more appropriate.

AFAIK, it's like you said about relative instruction limits, but also because
some predictors don't predict past a certain distance. So performance as well.
Not sure the reasons for each arch, or if they all apply for BPF JIT. There seem
to be 8 by my count, that have a dedicated module space for some reason.


So before refactoring the module alloc/free routines to accommodate
BPF, I'd like to take one step back and assess whether it wouldn't be
more appropriate to have a separate bpf_alloc/free API, which could be
totally separate from module alloc/free if the arch permits it.


I am not a BPF JIT expert unfortunately, hopefully someone more authoritative
will chime in. I only ran into this because I was trying to increase
randomization for the module space and wanted to find out how many allocations
needed to be supported.

I'd guess though, that BPF JIT is just assuming that there will be some arch
specific constraints about where text can be placed optimally and they would
already be taken into account in the module space allocator.

If there are no constraints for some arch, I'd wonder why not just update its
module_alloc to use the whole space available. What exactly are the constraints
for arm64 for normal modules?



Relative branches and the interactions with KAsan.

We just fixed something similar in the kprobes code: it was using
RWX-mapped module memory to store kprobed instructions, and we
replaced that with a simple vmalloc_exec() [and code to remap it
read-only], which was possible given that relative branches are always
emulated by arm64 kprobes.

So it depends on whether BPF code needs to be in relative branching
range from the calling code, and whether the BPF code itself is
emitted using relative branches into other parts of the code.


It seems fine to me for architectures to have the option of solving this a
different way. If potentially the rlimit ends up being the best solution for
some architectures though, do you think this refactor (pretty close to just a
name change) is that intrusive?

I guess it could be just a BPF JIT per user limit and not touch module space,
but I thought the module space limit was a more general solution as that is the
actual limited resource.



I think it is wrong to conflate the two things. Limiting the number of
BPF allocations and the limiting number of module allocations are two
separate things, and the fact that BPF reuses module_alloc() out of
convenience does not mean a single rlimit for both is appropriate.


Hm, I think Ard has a good point. AFAIK, and correct me if I'm wrong,
users of module_alloc() i.e. kprobes, ftrace, bpf, seem to use it
because it is an easy way to obtain executable kernel memory (and
depending on the needs of the architecture, being additionally
reachable via relative branches) during runtime. The side effect is
that all these users share the "module" memory space, even though this
memory region is not exclusively used by modules (well, personally I
think it technically should be, because seeing module_alloc() usage
outside of the module loader is kind of a misuse of the module API and
it's confusing for people who don't know the reason behind its usage
outside of the module loader).

Right now I'm not sure if it makes sense to impose a blanket limit on
all module_alloc() allocations when the real motivation behind the
rlimit is related to BPF, i.e., to stop unprivileged users from
hogging up all the vmalloc space for modules with JITed BPF filters.
So the rlimit has more to do with limiting the memory usage of BPF
filters than it has to do with modules themselves.

I think Ard's suggestion of having a separate bpf_alloc/free API makes
a lot of sense if we want to keep track of bpf-related allocations
(and then the rlimit would be enforced for those). Maybe part of the
module mapping space could be carved out for bpf filters (e.g. have
BPF_VADDR, BPF_VSIZE, etc like how we have it for modules), or
continue sharing the region but explicitly define a separate bpf_alloc
API, depending on an architecture's needs. What do people think?

Thanks,

Jessica

Re: [PATCH] arm64/module: use mod->klp_info section header information

[Jessica Yu]

+++ Miroslav Benes [25/10/18 11:00 +0200]:

On Thu, 25 Oct 2018, Petr Mladek wrote:


On Tue 2018-10-23 19:55:54, Jessica Yu wrote:
> The arm64 module loader keeps a pointer into info->sechdrs to keep track
> of section header information for .plt section(s). A pointer to the
> relevent section header (struct elf64_shdr) in info->sechdrs is stored
> in mod->arch.{init,core}.plt. This pointer may be accessed while
> applying relocations in apply_relocate_add() for example. And unlike
> normal modules, livepatch modules can call apply_relocate_add() after
> module load. But the info struct (and therefore info->sechdrs) gets
> freed at the end of load_module() and so mod->arch.{init,core}.plt
> becomes an invalid pointer after the module is done loading.
>
> Luckily, livepatch modules already keep a copy of Elf section header
> information in mod->klp_info. So make sure livepatch modules on arm64
> have access to the section headers in klp_info and set
> mod->arch.{init,core}.plt to the appropriate section header in
> mod->klp_info so that they can call apply_relocate_add() even after
> module load.
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f475f30eed8c..f3ac04cc9fc3 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3367,6 +3367,8 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
>
> static int post_relocation(struct module *mod, const struct load_info *info)
> {
> +  int err;
> +
>/* Sort exception table now relocations are done. */
>sort_extable(mod->extable, mod->extable + mod->num_exentries);
>
> @@ -3377,8 +3379,18 @@ static int post_relocation(struct module *mod, const 
struct load_info *info)
>/* Setup kallsyms-specific fields. */
>add_kallsyms(mod, info);
>
> +  if (is_livepatch_module(mod)) {
> +  err = copy_module_elf(mod, info);
> +  if (err < 0)
> +  return err;
> +  }
> +
>/* Arch-specific module finalizing. */
> -  return module_finalize(info->hdr, info->sechdrs, mod);
> +  err = module_finalize(info->hdr, info->sechdrs, mod);
> +  if (err < 0)

if (err < 0 && is_livepatch_module(mod))


Ah, right.


> +  free_module_elf(mod);
> +
> +  return err;
> }

Also we need to free the copied stuff in load_module() when
anything called after post_relocation() fails. I think
that the following would work:

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3823,6 +3823,8 @@ static int load_module(struct load_info *info, const char 
__user *uargs,
kfree(mod->args);
  free_arch_cleanup:
module_arch_cleanup(mod);
+   if (is_livepatch_module(mod))
+   free_module_elf(mod);
  free_modinfo:
free_modinfo(mod);
  free_unload:


Yes, we need to free it somewhere and I missed it. free_arch_cleanup seems
to be the correct place.


Good catches, thank you both!


But I suggest to just move copy_module_elf() up and keep
calling it from load_module() directly. It would make
the error handling more clear.


Unfortunately it is not that simple. arm64's module_finalize() uses
mod->klp_info with the patch, so copy_module_elf() must be called before.
We could move module_finalize() from post_relocation() to load_module()
and place copy_module_elf() between those two, but I don't know. That's up
to Jessica.


Yeah, it's a stylistic preference - will shuffle those calls around
and see what looks best. v2 to come shortly.

Thanks!

Jessica

[PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules

[Jessica Yu]

The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.

Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.

Signed-off-by: Jessica Yu 
---

v2:
 - fix missing free_module_elf() in error path
 - move copy_module_elf() and module_finalize() out of post_relocation()
   to make error handling more clear
 - add braces to if-else block in arm64 module_frob_arch_sections()

arch/arm64/include/asm/module.h |  1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c  | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr   *plt;
int plt_num_entries;
int plt_max_entries;
+   int plt_shndx;
};

struct mod_arch_specific {
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..851311ffd427 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -210,16 +210,23 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr 
*sechdrs,
 * entries. Record the symtab address as well.
 */
for (i = 0; i < ehdr->e_shnum; i++) {
-   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) {
mod->arch.core.plt = sechdrs + i;
-   else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
+   /*
+* Keep the section index for the .plt section for
+* livepatching. Note that .init.plt is irrelevant to
+* livepatch, so only the shndx for .plt is saved.
+*/
+   mod->arch.core.plt_shndx = i;
+   } else if (!strcmp(secstrings + sechdrs[i].sh_name, 
".init.plt")) {
mod->arch.init.plt = sechdrs + i;
-   else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
+   } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
 !strcmp(secstrings + sechdrs[i].sh_name,
-".text.ftrace_trampoline"))
+".text.ftrace_trampoline")) {
tramp = sechdrs + i;
-   else if (sechdrs[i].sh_type == SHT_SYMTAB)
+   } else if (sechdrs[i].sh_type == SHT_SYMTAB) {
syms = (Elf64_Sym *)sechdrs[i].sh_addr;
+   }
}

if (!mod->arch.core.plt || !mod->arch.init.plt) {
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index dd23655fda3a..490e56070a7e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr,
#endif
}

+#ifdef CONFIG_LIVEPATCH
+   /*
+* For livepatching, switch to the saved section header info for .plt
+* stored in mod->klp_info. This is needed so that livepatch is able to
+* call apply_relocate_add() after patch module load.
+*/
+   if (is_livepatch_module(me))
+   me->arch.core.plt = me->klp_info->sechdrs + 
me->arch.core.plt_shndx;
+#endif
+
return 0;
}
diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..611f4fe64370 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3365,7 +3365,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
return 0;
}

-static int post_relocation(struct module *mod, const struct load_info *info)
+static void post_relocation(struct module *mod, const struct load_info *info)
{
/* Sort exception table now relocations are done. */
sort_extable(mod->exta

Re: [PATCH v3 3/4] arm64: implement live patching

[Jessica Yu]

+++ Miroslav Benes [17/10/18 15:39 +0200]:

On Mon, 1 Oct 2018, Torsten Duwe wrote:


Based on ftrace with regs, do the usual thing. Also allocate a
task flag for whatever consistency handling will be used.
Watch out for interactions with the graph tracer.


Similar to what Mark wrote about 2/4, I'd appreciate a better commit log.
Could you explain traditional "what/why/how", please?


Signed-off-by: Torsten Duwe 

--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -119,6 +119,7 @@ config ARM64
select HAVE_GENERIC_DMA_COHERENT
select HAVE_HW_BREAKPOINT if PERF_EVENTS
select HAVE_IRQ_TIME_ACCOUNTING
+   select HAVE_LIVEPATCH
select HAVE_MEMBLOCK
select HAVE_MEMBLOCK_NODE_MAP if NUMA
select HAVE_NMI
@@ -1349,4 +1350,6 @@ if CRYPTO
 source "arch/arm64/crypto/Kconfig"
 endif

+source "kernel/livepatch/Kconfig"
+
 source "lib/Kconfig"
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas
 #define TIF_FOREIGN_FPSTATE3   /* CPU's FP state is not current's */
 #define TIF_UPROBE 4   /* uprobe breakpoint or singlestep */
 #define TIF_FSCHECK5   /* Check FS is USER_DS on return */
+#define TIF_PATCH_PENDING  6
 #define TIF_NOHZ   7
 #define TIF_SYSCALL_TRACE  8
 #define TIF_SYSCALL_AUDIT  9
@@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas
 #define _TIF_NEED_RESCHED  (1 << TIF_NEED_RESCHED)
 #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME)
 #define _TIF_FOREIGN_FPSTATE   (1 << TIF_FOREIGN_FPSTATE)
+#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING)
 #define _TIF_NOHZ  (1 << TIF_NOHZ)
 #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT)
@@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas

 #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
 _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
-_TIF_UPROBE | _TIF_FSCHECK)
+_TIF_UPROBE | _TIF_FSCHECK | \
+_TIF_PATCH_PENDING)


Could you add a note to the changelog what this means? My ability to read
arm64 entry.S is very limited, but I can see that _TIF_WORK_MASK is
process in a syscall exit and irq return paths. That's good. It is also
called (via "b ret_to_user") in a couple of different places (el0_*
labels). I guess those are returns from exception handling. A comment
about it in the changelog would be appreciated.


 #define _TIF_SYSCALL_WORK  (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \
--- /dev/null
+++ b/arch/arm64/include/asm/livepatch.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * livepatch.h - arm64-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2016,2018 SUSE
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see .
+ */
+#ifndef _ASM_ARM64_LIVEPATCH_H
+#define _ASM_ARM64_LIVEPATCH_H
+
+#include 
+#include 


I think that only

#include 

is in fact needed, because of "struct pt_regs".


Ad relocations. I checked that everything in struct mod_arch_specific
stays after the module is load. Both core and init get SHF_ALLOC set
(mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is
important because apply_relocate_add() may use those sections
through module_emit_plt_entry() call.


Yes, it looks like the needed .plt sections will remain in module
memory. However, I think I found a slight issue... :/

In module_frob_arch_sections(), mod->arch.core.plt is set to an offset
within info->sechdrs:

if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
mod->arch.core.plt = sechdrs + i;

sechdrs is from info->sechdrs, which is freed at the end of
load_module() via free_copy(). So although the relevant plt section(s)
are in module memory, mod->arch.core.plt will point to invalid memory
after info is freed. In other words, the section (.plt) *is* in memory
but the section header (struct elf64_shdr) containing section metadata
like sh_addr, sh_size etc., is not.

But we have mod->klp_info->sechdrs (which is a copy of the section
headers) for this very reason. It is initialized only

Re: [PATCH v3 3/4] arm64: implement live patching

[Jessica Yu]

+++ Miroslav Benes [19/10/18 13:59 +0200]:

On Thu, 18 Oct 2018, Jessica Yu wrote:


+++ Miroslav Benes [17/10/18 15:39 +0200]:
>On Mon, 1 Oct 2018, Torsten Duwe wrote:
>
>Ad relocations. I checked that everything in struct mod_arch_specific
>stays after the module is load. Both core and init get SHF_ALLOC set
>(mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is
>important because apply_relocate_add() may use those sections
>through module_emit_plt_entry() call.

Yes, it looks like the needed .plt sections will remain in module
memory. However, I think I found a slight issue... :/

In module_frob_arch_sections(), mod->arch.core.plt is set to an offset
within info->sechdrs:

if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
mod->arch.core.plt = sechdrs + i;

sechdrs is from info->sechdrs, which is freed at the end of
load_module() via free_copy(). So although the relevant plt section(s)
are in module memory, mod->arch.core.plt will point to invalid memory
after info is freed. In other words, the section (.plt) *is* in memory
but the section header (struct elf64_shdr) containing section metadata
like sh_addr, sh_size etc., is not.

But we have mod->klp_info->sechdrs (which is a copy of the section
headers) for this very reason. It is initialized only at the very end
of load_module(). I don't think copy_module_elf() is dependent on
anything, so it can just be moved earlier.

Note that we cannot set mod->arch.core.plt to mod->klp_info->sechdrs + i
during module_frob_arch_sections() because it is still too early, none
of the module sections have been copied and none of their sh_addr's
have been set to their final locations as this is all happening before
move_module() is called. So we can use a module_finalize() function
for arm64 to switch the mod->arch.core.plt to use the klp_info section
headers.


Thanks for the analysis. You seem to be right.


Maybe this will be more clear in the example fix below (completely
untested and unreviewed!):

diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 97d0ef12e2ff..150afc29171b 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr   *plt;
int plt_num_entries;
int plt_max_entries;
+   int plt_shndx;
};

struct mod_arch_specific {
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..c23cef8f0165 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -196,6 +196,21 @@ static unsigned int count_plts(Elf64_Sym *syms,
Elf64_Rela *rela, int num,
return ret;
}

+int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
+   struct module *mod)
+{
+   /*
+* Livepatch modules need to keep access to section headers to apply
+* relocations. Note mod->klp_info should have already been
initialized
+* and all section sh_addr's should have their final addresses by the
+* time module_finalize() is called.
+*/
+   if (is_livepatch_module(mod))
+   mod->arch.core.plt = mod->klp_info->sechdrs +
mod->arch.core.plt_shndx;
+
+   return 0;
+}


There is already module_finalize() in arch/arm64/kernel/module.c, so this
should probably go there.


Ah yeah, I missed that :) Yep, that would be where it'd go.


If I am not mistaken, we do not care for arch.init.plt in livepatch. Is
that correct?


I do not believe patching of __init functions is supported (right?) So
we do not need to keep arch.init.plt alive post-module-load.


int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
  char *secstrings, struct module *mod)
{
@@ -210,11 +225,13 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
*sechdrs,
 * entries. Record the symtab address as well.
 */
for (i = 0; i < ehdr->e_shnum; i++) {
-   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+   if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) {
mod->arch.core.plt = sechdrs + i;
-   else if (!strcmp(secstrings + sechdrs[i].sh_name,
".init.plt"))
+   mod->arch.core.plt_shndx = i;
+   } else if (!strcmp(secstrings + sechdrs[i].sh_name,
".init.plt")) {
mod->arch.init.plt = sechdrs + i;
-   else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
+   mod->arch.init.plt_shndx = i;


It is initialized here, but that's not necessarily a bad thing.


I think I added this line for consistency, but I actually don't think
it is neede

Re: [PATCH v2 2/2] certs: Add support for using elliptic curve keys for signing modules

[Jessica Yu]

+++ Stefan Berger [08/04/21 11:24 -0400]:

Add support for using elliptic curve keys for signing modules. It uses
a NIST P384 (secp384r1) key if the user chooses an elliptic curve key
and will have ECDSA support built into the kernel.

Note: A developer choosing an ECDSA key for signing modules should still
delete the signing key (rm certs/signing_key.*) when building an older
version of a kernel that only supports RSA keys. Unless kbuild automati-
cally detects and generates a new kernel module key, ECDSA-signed kernel
modules will fail signature verification.

Signed-off-by: Stefan Berger 

---
v2:
 - check for ECDSA key by id-ecPublicKey from output line
   'Public Key Algorithm: id-ecPublicKey'.
---
certs/Kconfig | 25 +
certs/Makefile|  9 +
crypto/asymmetric_keys/pkcs7_parser.c |  4 
3 files changed, 38 insertions(+)

diff --git a/certs/Kconfig b/certs/Kconfig
index 48675ad319db..6f8337874ae0 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -15,6 +15,31 @@ config MODULE_SIG_KEY
 then the kernel will automatically generate the private key and
 certificate as described in 
Documentation/admin-guide/module-signing.rst

+choice
+   prompt "Type of module signing key to be generated"
+   default MODULE_SIG_KEY_TYPE_RSA
+   help
+The type of module signing key type to generate. This option
+does not apply if a #PKCS11 URI is used.
+
+config MODULE_SIG_KEY_TYPE_RSA
+   bool "RSA"
+   depends on MODULE_SIG || IMA_APPRAISE_MODSIG
+   help
+Use an RSA key for module signing.
+
+config MODULE_SIG_KEY_TYPE_ECDSA
+   bool "ECDSA"
+   select CRYPTO_ECDSA
+   depends on MODULE_SIG || IMA_APPRAISE_MODSIG
+   help
+Use an elliptic curve key (NIST P384) for module signing.
+
+Note: Remove all ECDSA signing keys, e.g. certs/signing_key.pem,
+when falling back to building Linux 5.11 and older kernels.
+
+endchoice
+
config SYSTEM_TRUSTED_KEYRING
bool "Provide system-wide ring of trusted keys"
depends on KEYS
diff --git a/certs/Makefile b/certs/Makefile
index f64bc89ccbf1..c2fabc288550 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -62,7 +62,15 @@ ifeq ($(CONFIG_MODULE_SIG_KEY),"certs/signing_key.pem")

X509TEXT=$(shell openssl x509 -in $(CONFIG_MODULE_SIG_KEY) -text)

+# Support user changing key type
+ifdef CONFIG_MODULE_SIG_KEY_TYPE_ECDSA
+keytype_openssl = -newkey ec -pkeyopt ec_paramgen_curve:secp384r1
+$(if $(findstring id-ecPublicKey,$(X509TEXT)),,$(shell rm -f 
$(CONFIG_MODULE_SIG_KEY)))
+endif
+
+ifdef CONFIG_MODULE_SIG_KEY_TYPE_RSA
$(if $(findstring rsaEncryption,$(X509TEXT)),,$(shell rm -f 
$(CONFIG_MODULE_SIG_KEY)))
+endif

$(obj)/signing_key.pem: $(obj)/x509.genkey
@$(kecho) "###"
@@ -77,6 +85,7 @@ $(obj)/signing_key.pem: $(obj)/x509.genkey
-batch -x509 -config $(obj)/x509.genkey \
-outform PEM -out $(obj)/signing_key.pem \
-keyout $(obj)/signing_key.pem \
+   $(keytype_openssl) \
$($(quiet)redirect_openssl)
@$(kecho) "###"
@$(kecho) "### Key pair generated."
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c 
b/crypto/asymmetric_keys/pkcs7_parser.c
index 967329e0a07b..2546ec6a0505 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypto/asymmetric_keys/pkcs7_parser.c
@@ -269,6 +269,10 @@ int pkcs7_sig_note_pkey_algo(void *context, size_t hdrlen,
ctx->sinfo->sig->pkey_algo = "rsa";
ctx->sinfo->sig->encoding = "pkcs1";
break;
+   case OID_id_ecdsa_with_sha256:
+   ctx->sinfo->sig->pkey_algo = "ecdsa";
+   ctx->sinfo->sig->encoding = "x962";
+   break;


Hi Stefan,

Does CONFIG_MODULE_SIG_KEY_TYPE_ECDSA have a dependency on MODULE_SIG_SHA256?
By default, MODULE_SIG_SHA1 is selected when CONFIG_MODULE_SIG is enabled.
I was doing some quick testing and found that when I enabled
MODULE_SIG_KEY_TYPE_ECDSA I get a "Unsupported pkey algo: 5" error on
module load, which goes away after fixing my config and selecting
MODULE_SIG_SHA256.

Thanks,

Jessica

Re: [PATCH v3 02/12] buildid: Stash away kernels build ID on init

[Jessica Yu]

+++ Stephen Boyd [30/03/21 20:05 -0700]:
[snipped]

diff --git a/lib/buildid.c b/lib/buildid.c
index 010ab0674cb9..b939bbc59233 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0

#include 
+#include 
#include 
#include 
#include 
@@ -171,3 +172,19 @@ int build_id_parse_buf(const void *buf, unsigned char 
*build_id, u32 buf_size)
{
return parse_build_id_buf(build_id, NULL, buf, buf_size);
}
+
+unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX] __ro_after_init;
+
+/**
+ * init_vmlinux_build_id - Get the running kernel's build ID
+ *
+ * Return: Running kernel's build ID
+ */


Hm, init_vmlinux_build_id() doesn't return anything, so this comment is
not accurate - maybe "Get the running kernel's build ID and store it in
vmlinux_build_id"?


+void __init init_vmlinux_build_id(void)
+{
+   extern const void __start_notes __weak;
+   extern const void __stop_notes __weak;
+   unsigned int size = &__stop_notes - &__start_notes;
+
+   build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
+}

Re: [PATCH v3 04/12] module: Add printk format to add module build ID to stacktraces

[Jessica Yu]

+++ Stephen Boyd [30/03/21 20:05 -0700]:
[snipped]

diff --git a/kernel/module.c b/kernel/module.c
index 30479355ab85..6f5bc1b046a5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -13,6 +13,7 @@
#include 
#include 
#include 
+#include 
#include 
#include 
#include 
@@ -2770,6 +2771,20 @@ static void add_kallsyms(struct module *mod, const 
struct load_info *info)
}
mod->core_kallsyms.num_symtab = ndst;
}
+
+static void init_build_id(struct module *mod, const struct load_info *info)
+{
+   const Elf_Shdr *sechdr;
+   unsigned int i;
+
+   for (i = 0; i < info->hdr->e_shnum; i++) {
+   sechdr = &info->sechdrs[i];
+   if (!sect_empty(sechdr) && sechdr->sh_type == SHT_NOTE &&
+   !build_id_parse_buf((void *)sechdr->sh_addr, mod->build_id,
+   sechdr->sh_size))
+   break;
+   }
+}


Why not just look for the .note.gnu.build-id section instead of trying
to parse each note section? Doesn't it always contain the build id? At
least the ld man page seems to suggest this section name should be
consistent.

Jessica

Re: [PATCH v3 0/3][RESEND] modsign enhancement

[Jessica Yu]

+++ Jia Zhang [24/03/18 10:59 +0800]:

This patch series allows to disable module validity enforcement
in runtime through the control switch located in securityfs.

In order to keep /sys/module/module/parameters/sig_enforce simple,
the disablement switch is located at
/sys/kernel/security/modsign/disable_enforce.

Assuming CONFIG_MODULE_SIG_FORCE=n, here are the instructions to
test this control switch.

# cat /sys/module/module/parameters/sig_enforce
N
# echo 1 > /sys/module/module/parameters/sig_enforce
# cat /sys/module/module/parameters/sig_enforce
Y
# echo -n 0 > no_sig_enforce
# openssl smime -sign -nocerts -noattr -binary -in no_sig_enforce \
   -inkey  -signer  -outform der \
   -out /sys/kernel/security/modsign/disable_enforce
# cat /sys/module/module/parameters/sig_enforce
N


I'm not convinced we need this. And neither the use case nor the
motivation is explained in the cover letter :-(

The way I see it - the only time you'd actually use this is in the
situation where you have *already* enabled sig_enforce, and then later
you change your mind - meaning you wanted to load unsigned modules
after all. And if you ever plan on loading unsigned modules, why would
you have enabled sig_enforce in the first place? If you want to keep
the option of loading unsigned modules, don't have sig_enforce or
CONFIG_MODULE_SIG_FORCE enabled.

[ CC'd Rusty in case he has some thoughts on this ]

Jessica

[GIT PULL] modules fix for v4.17-rc3

[Jessica Yu]

Hi Linus,

Please pull below for a modules fix for 4.17-rc3.

Thanks,

Jessica

---
The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:

 Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)

are available in the git repository at:

 ssh://g...@gitolite.kernel.org/pub/scm/linux/kernel/git/jeyu/linux.git 
tags/modules-for-v4.17-rc3

for you to fetch changes up to be71eda5383faa663efdba9ef54a6b8255e3c7f0:

 module: Fix display of wrong module .text address (2018-04-18 22:59:46 +0200)


Modules fix for v4.17-rc3

- Fix display of module section addresses in sysfs, which were getting
hashed with %pK and breaking tools like perf.

Signed-off-by: Jessica Yu 


Thomas Richter (1):
 module: Fix display of wrong module .text address

kernel/module.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Re: /proc/kallsyms shows undefined symbols for livepatch modules

[Jessica Yu]

+++ Josh Poimboeuf [02/06/18 12:32 -0500]:

Hi Jessica,

I found a bug:

 [root@f25 ~]# modprobe livepatch-sample
 [root@f25 ~]# grep ' u ' /proc/kallsyms
 81161080 u klp_enable_patch[livepatch_sample]
 81a01800 u __fentry__  [livepatch_sample]
 81161250 u klp_unregister_patch[livepatch_sample]
 81161870 u klp_register_patch  [livepatch_sample]
 8131f0b0 u seq_printf  [livepatch_sample]

Notice that livepatch modules' undefined symbols are showing up in
/proc/kallsyms.  This can confuse klp_find_object_symbol() which can
cause subtle bugs in livepatch.

I stared at the module kallsyms code for a bit, but I don't see the bug.
Maybe it has something to do with how we save the symbol table in
copy_module_elf().  Any ideas?


Hi Josh!

This is because we preserve the entire symbol table for livepatch
modules, including the SHN_UNDEF symbols. IIRC, this is so that we can
still apply relocations properly with apply_relocate_add() after a
to-be-patched object is loaded. Normally we don't save these SHN_UNDEF
symbols for modules so they do not appear in /proc/kallsyms.

I think I see how this can cause bugs, since if we're searching for
seq_printf we want the real seq_printf and not the undef symbol from
the livepatch module. But I thought we renamed these symbols with some
.klp prefix? Or is the livepatch bug you're running into something else?

Thanks,

Jessica

Re: /proc/kallsyms shows undefined symbols for livepatch modules

[Jessica Yu]

+++ Jessica Yu [04/06/18 10:05 +0200]:

+++ Josh Poimboeuf [02/06/18 12:32 -0500]:

Hi Jessica,

I found a bug:

[root@f25 ~]# modprobe livepatch-sample
[root@f25 ~]# grep ' u ' /proc/kallsyms
81161080 u klp_enable_patch [livepatch_sample]
81a01800 u __fentry__   [livepatch_sample]
81161250 u klp_unregister_patch [livepatch_sample]
81161870 u klp_register_patch   [livepatch_sample]
8131f0b0 u seq_printf   [livepatch_sample]

Notice that livepatch modules' undefined symbols are showing up in
/proc/kallsyms.  This can confuse klp_find_object_symbol() which can
cause subtle bugs in livepatch.

I stared at the module kallsyms code for a bit, but I don't see the bug.
Maybe it has something to do with how we save the symbol table in
copy_module_elf().  Any ideas?


Hi Josh!

This is because we preserve the entire symbol table for livepatch
modules, including the SHN_UNDEF symbols. IIRC, this is so that we can
still apply relocations properly with apply_relocate_add() after a
to-be-patched object is loaded. Normally we don't save these SHN_UNDEF
symbols for modules so they do not appear in /proc/kallsyms.


Hm, if having the full symtab in kallsyms is causing trouble, one
possibility would be to just have the module kallsyms code simply
skip/ignore undef symbols. That's what we technically do for normal
modules anyway (we normally cut undef syms out of the symtab). Haven't
tested this idea but does that sound like it'd help?

Thanks,

Jessica