libkmod-elf: resolve CRC if module is built with MODULE_REL_CRCS

PATCH In review Yauheni Kaliuta 3 participants 6 messages Jul 19, 2017 – Aug 8, 2017

lore

Share & tools

lore

Yauheni Kaliuta

author

2017-07-19 14:53 UTC #0 lore raw

PATCH 1/1 libkmod-elf: resolve CRC if module is built with MODULE_REL_CRCS

Normally exported symbol's crc is stored as absolute (SHN_ABS) value of special named symbol __crc_<symbol name>.

When the kernel and modules are built with the config option CONFIG_MODULE_REL_CRCS, all the CRCs are put in a special section and the __crc_<symbol name> symbols values are offsets in the section. See patch description of the commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56067812d5b0e737ac2063e94a50f76b810d6ca3

Add kmod support of this configuration.

Signed-off-by Yauheni Kaliuta <[email protected]>

---
 libkmod/libkmod-elf.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

libkmod/libkmod-elf.c +29 -1

index 90da89aebbaf..ef4a8a3142a1 100644
@@ -747,6 +747,31 @@ static inline uint8_t kmod_symbol_bind_from_elf(uint8_t elf_value)
 	}
 }
 
+static uint64_t kmod_elf_resolve_crc(const struct kmod_elf *elf, uint64_t crc, uint16_t shndx)
+{
+	int err;
+	uint64_t off, size;
+	uint32_t nameoff;
+
+	if (shndx == SHN_ABS || shndx == SHN_UNDEF)
+		return crc;
+
+	err = elf_get_section_info(elf, shndx, &off, &size, &nameoff);
+	if (err < 0) {
+		ELFDBG("Cound not find section index %"PRIu16" for crc", shndx);
+		return (uint64_t)-1;
+	}
+
+	if (crc > (size - sizeof(uint32_t))) {
+		ELFDBG("CRC offset %"PRIu64" is too big, section %"PRIu16" size is %"PRIu64"\n",
+		       crc, shndx, size);
+		return (uint64_t)-1;
+	}
+
+	crc = elf_get_uint(elf, off + crc, sizeof(uint32_t));
+	return crc;
+}
+
 /* array will be allocated with strings in a single malloc, just free *array */
 int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **array)
 {

@@ -830,6 +855,7 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
 		uint32_t name_off;
 		uint64_t crc;
 		uint8_t info, bind;
+		uint16_t shndx;
 
 #define READV(field)							\
 		elf_get_uint(elf, sym_off + offsetof(typeof(*s), field),\

@@ -839,11 +865,13 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
 			name_off = READV(st_name);
 			crc = READV(st_value);
 			info = READV(st_info);
+			shndx = READV(st_shndx);
 		} else {
 			Elf64_Sym *s;
 			name_off = READV(st_name);
 			crc = READV(st_value);
 			info = READV(st_info);
+			shndx = READV(st_shndx);
 		}
 #undef READV
 		name = elf_get_mem(elf, str_off + name_off);

@@ -856,7 +884,7 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
 		else
 			bind = ELF64_ST_BIND(info);
 
-		a[count].crc = crc;
+		a[count].crc = kmod_elf_resolve_crc(elf, crc, shndx);
 		a[count].bind = kmod_symbol_bind_from_elf(bind);
 		a[count].symbol = itr;
 		slen = strlen(name);

Signature · 1 line

2.14.0.rc0

Lucas De Marchi

2017-07-19 17:51 UTC #1 lore raw

Quoted 76 lines · Yauheni Kaliuta

Normally exported symbol's crc is stored as absolute (SHN_ABS) value of special named symbol __crc_<symbol name>.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56067812d5b0e737ac2063e94a50f76b810d6ca3

Add kmod support of this configuration.

Signed-off-by Yauheni Kaliuta <[email protected]>

---
 libkmod/libkmod-elf.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

libkmod/libkmod-elf.c +28 -0

index 90da89aebbaf..ef4a8a3142a1 100644
@@ -747,6 +747,31 @@ static inline uint8_t kmod_symbol_bind_from_elf(uint8_t elf_value)
        }
 }
 
+static uint64_t kmod_elf_resolve_crc(const struct kmod_elf *elf, uint64_t crc, uint16_t shndx)
+{
+       int err;
+       uint64_t off, size;
+       uint32_t nameoff;
+
+       if (shndx == SHN_ABS || shndx == SHN_UNDEF)
+               return crc;
+
+       err = elf_get_section_info(elf, shndx, &off, &size, &nameoff);
+       if (err < 0) {
+               ELFDBG("Cound not find section index %"PRIu16" for crc", shndx);
+               return (uint64_t)-1;
+       }
+
+       if (crc > (size - sizeof(uint32_t))) {
+               ELFDBG("CRC offset %"PRIu64" is too big, section %"PRIu16" size is %"PRIu64"\n",
+                      crc, shndx, size);
+               return (uint64_t)-1;
+       }
+
+       crc = elf_get_uint(elf, off + crc, sizeof(uint32_t));
+       return crc;
+}
+
 /* array will be allocated with strings in a single malloc, just free *array */
 int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **array)
 {

@@ -830,6 +855,7 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
                uint32_t name_off;
                uint64_t crc;
                uint8_t info, bind;
+               uint16_t shndx;
 
 #define READV(field)                                                   \
                elf_get_uint(elf, sym_off + offsetof(typeof(*s), field),\

@@ -839,11 +865,13 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
                        name_off = READV(st_name);
                        crc = READV(st_value);
                        info = READV(st_info);
+                       shndx = READV(st_shndx);
                } else {
                        Elf64_Sym *s;
                        name_off = READV(st_name);
                        crc = READV(st_value);
                        info = READV(st_info);
+                       shndx = READV(st_shndx);
                }
 #undef READV
                name = elf_get_mem(elf, str_off + name_off);

libkmod/libkmod-elf.c int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar

Quoted 9 lines

libkmod/libkmod-elf.c +1 -2

@@ -856,7 +884,7 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
                else
                        bind = ELF64_ST_BIND(info);
 
-               a[count].crc = crc;
+               a[count].crc = kmod_elf_resolve_crc(elf, crc, shndx);
                a[count].bind = kmod_symbol_bind_from_elf(bind);
                a[count].symbol = itr;
                slen = strlen(name);
--

LGTM. I'll give this a try and apply. Do you think it's possible to build an out-of-tree module with this option so we can add one to the testsuite?

Lucas De Marchi

Yauheni Kaliuta

author

2017-07-19 19:01 UTC #2 lore raw

Hi, Lucas!

Quoted 14 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Lucas De Marchi
Quoted 11 lines · Yauheni Kaliuta
Normally exported symbol's crc is stored as absolute (SHN_ABS) value of special named symbol __crc_<symbol name>.
When the kernel and modules are built with the config option CONFIG_MODULE_REL_CRCS, all the CRCs are put in a special section and the __crc_<symbol name> symbols values are offsets in the section. See patch description of the commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56067812d5b0e737ac2063e94a50f76b810d6ca3
Add kmod support of this configuration.

[...]

Quoted 1 line

LGTM. I'll give this a try and apply.

Thanks! I would also like to get some feedback from the kernel feature author, Ard Biesheuvel, that this does not miss other usecases.

Quoted 2 lines

Do you think it's possible to build an out-of-tree module with this option so we can add one to the testsuite?

I'll check what I can do.

The problem is seen if you run depmod with -e -E, which I guess is not used too often.

Signature · 2 lines

WBR,
Yauheni Kaliuta

Ard Biesheuvel

Reviewed-by

2017-07-19 19:10 UTC #3 lore raw

Quoted 15 lines · Yauheni Kaliuta

Hi, Lucas!
Quoted 13 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Lucas De Marchi
Quoted 10 lines · Yauheni Kaliuta
Normally exported symbol's crc is stored as absolute (SHN_ABS) value of special named symbol __crc_<symbol name>.
When the kernel and modules are built with the config option CONFIG_MODULE_REL_CRCS, all the CRCs are put in a special section and the __crc_<symbol name> symbols values are offsets in the section. See patch description of the commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56067812d5b0e737ac2063e94a50f76b810d6ca3

Quoted 9 lines

Quoted 1 line
Quoted 1 line
Add kmod support of this configuration.
[...]
Quoted 1 line
LGTM. I'll give this a try and apply.
Thanks! I would also like to get some feedback from the kernel feature author, Ard Biesheuvel, that this does not miss other usecases.

Hi all,

The patch looks correct to me. My only comment is that kmod_elf_resolve_crc() should probably return a uint32_t instead, given the size of a CRC32.

In any case,

Reviewed-by Ard Biesheuvel <[email protected]>

Thanks,
Ard.

Quoted 11 lines

Quoted 2 lines
Do you think it's possible to build an out-of-tree module with this option so we can add one to the testsuite?
I'll check what I can do.
The problem is seen if you run depmod with -e -E, which I guess is not used too often.
--
WBR,
Yauheni Kaliuta

Yauheni Kaliuta

author

2017-07-19 19:30 UTC #4 lore raw

Hi, Ard!

Quoted 24 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Ard Biesheuvel
Quoted 21 lines · Yauheni Kaliuta
Hi, Lucas!
Quoted 14 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Lucas De Marchi
Quoted 11 lines · Yauheni Kaliuta
Normally exported symbol's crc is stored as absolute (SHN_ABS) value of special named symbol __crc_<symbol name>.
When the kernel and modules are built with the config option CONFIG_MODULE_REL_CRCS, all the CRCs are put in a special section and the __crc_<symbol name> symbols values are offsets in the section. See patch description of the commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=56067812d5b0e737ac2063e94a50f76b810d6ca3
Add kmod support of this configuration.
[...]
Quoted 1 line
LGTM. I'll give this a try and apply.

Quoted 7 lines

Quoted 3 lines
Thanks! I would also like to get some feedback from the kernel feature author, Ard Biesheuvel, that this does not miss other usecases.
The patch looks correct to me. My only comment is that kmod_elf_resolve_crc() should probably return a uint32_t instead, given the size of a CRC32.

Well, it's a specific of the internal libkmod implementation. Both struct kmod_modversion::crc and return value of elf_get_uint() are uint64_t, but in the patch elf_get_uint() reads correct sizeof(uint32_t) value from the ELF.

Quoted 3 lines

In any case,
Reviewed-by Ard Biesheuvel <[email protected]>

Thanks a lot!

Signature · 2 lines

WBR,
Yauheni Kaliuta

Yauheni Kaliuta

author

2017-08-08 21:40 UTC #5 lore raw

RFC 1/1 resolve CRC test. Was: Re: [PATCH] libkmod-elf: resolve CRC if module is built with MODULE_REL_CRCS

In reply to Yauheni Kaliuta

Hi, Lucas!

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines · Yauheni Kaliuta
On Wed, 19 Jul 2017 10:51:44 -0700, Lucas De Marchi wrote:

[...]

Quoted 4 lines

Quoted 2 lines
Do you think it's possible to build an out-of-tree module with this option so we can add one to the testsuite?
I'll check what I can do.

Some ideas below. I reused the existing "foo" modules with their dependencies. Possible problem is that it requires the host kernel to be built with MODVERSIONS enabled (not the case for Fedora).

---
 testsuite/module-playground/Makefile               | 10 +++++++-
 testsuite/populate-modules.sh                      |  4 +++
 .../test-depmod/relocated-crc/boot/symvers         |  0
 .../lib/modules/4.4.4/correct-modules.dep          |  4 +++
 .../lib/modules/4.4.4/modules.builtin              |  0
 .../relocated-crc/lib/modules/4.4.4/modules.order  |  0
 .../test-depmod/relocated-crc/output.err           | 10 ++++++++
 testsuite/test-depmod.c                            | 30 ++++++++++++++++++++++
 8 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 testsuite/rootfs-pristine/test-depmod/relocated-crc/boot/symvers
 create mode 100644 testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/correct-modules.dep
 create mode 100644 testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/modules.builtin
 create mode 100644 testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/modules.order
 create mode 100644 testsuite/rootfs-pristine/test-depmod/relocated-crc/output.err

testsuite/module-playground/Makefile +9 -1

index e6045b0..f12f301 100644
@@ -11,6 +11,10 @@ obj-m += mod-foo-b.o
 obj-m += mod-foo-c.o
 obj-m += mod-foo.o
 
+# predictable relocated/non-relocated symbols crc
+mod-foo-a-rel-y := mod-foo-a.o
+mod-foo-b-nonrel-y := mod-foo-b.o
+
 # mod-loop: create loops in dependencies:
 # 1) mod-loop-a  ->  mod-loop-b -> mod-loop-c -> mod-loop-a
 #     |-> mod-loop-f    |-> mod-loop-f

@@ -63,8 +67,12 @@ mod-simple-%.ko: mod-simple-%.c Makefile.arch
 	$(MAKE) KDIR=$(KDIR_$(arch)) ARCH=$(arch) CROSS_COMPILE=$(CROSS_COMPILE_$(arch)) -f Makefile.arch
 
 ifeq ($(FAKE_BUILD),0)
-modules:
+modules: rel-modules
 	$(MAKE) -C $(KDIR) M=$$PWD
+
+rel-modules:
+	$(MAKE) -C $(KDIR) M=$$PWD CONFIG_MODULE_REL_CRCS=y obj-m=mod-foo-a-rel.o
+	$(MAKE) -C $(KDIR) M=$$PWD CONFIG_MODULE_REL_CRCS=n obj-m=mod-foo-b-nonrel.o
 else
 modules:
 	@echo "  CP       cache/*.ko"

testsuite/populate-modules.sh +4 -0

index 3ac92ee..f6f71fa 100755
@@ -28,6 +28,10 @@ map=(
     ["test-depmod/search-order-external-last/lib/modules/4.4.4/foo/"]="mod-simple.ko"
     ["test-depmod/search-order-external-last/lib/modules/4.4.4/foobar/"]="mod-simple.ko"
     ["test-depmod/search-order-external-last/lib/modules/external/"]="mod-simple.ko"
+    ["test-depmod/relocated-crc/lib/modules/4.4.4/kernel/mod-foo-a-rel.ko"]="mod-foo-a-rel.ko"
+    ["test-depmod/relocated-crc/lib/modules/4.4.4/kernel/mod-foo-b-nonrel.ko"]="mod-foo-b-nonrel.ko"
+    ["test-depmod/relocated-crc/lib/modules/4.4.4/kernel/mod-foo-c.ko"]="mod-foo-c.ko"
+    ["test-depmod/relocated-crc/lib/modules/4.4.4/kernel/mod-foo.ko"]="mod-foo.ko"
     ["test-dependencies/lib/modules/4.0.20-kmod/kernel/fs/foo/"]="mod-foo-b.ko"
     ["test-dependencies/lib/modules/4.0.20-kmod/kernel/"]="mod-foo-c.ko"
     ["test-dependencies/lib/modules/4.0.20-kmod/kernel/lib/"]="mod-foo-a.ko"

testsuite/rootfs-pristine/test-depmod/relocated-crc/boot/symvers new file +0 -0

new file mode 100644
index 0000000..e69de29

testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/correct-modules.dep new file +4 -0

new file mode 100644
index 0000000..303060c
@@ -0,0 +1,4 @@
+kernel/mod-foo-c.ko:
+kernel/mod-foo-b-nonrel.ko:
+kernel/mod-foo.ko: kernel/mod-foo-b-nonrel.ko kernel/mod-foo-a-rel.ko kernel/mod-foo-c.ko
+kernel/mod-foo-a-rel.ko:

testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/modules.builtin new file +0 -0

new file mode 100644
index 0000000..e69de29

testsuite/rootfs-pristine/test-depmod/relocated-crc/lib/modules/4.4.4/modules.order new file +0 -0

new file mode 100644
index 0000000..e69de29

testsuite/rootfs-pristine/test-depmod/relocated-crc/output.err new file +10 -0

new file mode 100644
index 0000000..d7e4fb0
@@ -0,0 +1,10 @@
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-c.ko needs unknown symbol __fentry__
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-c.ko needs unknown symbol printk
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-c.ko needs unknown symbol module_layout
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-b-nonrel.ko needs unknown symbol __fentry__
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-b-nonrel.ko needs unknown symbol printk
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-b-nonrel.ko needs unknown symbol module_layout
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo.ko needs unknown symbol module_layout
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-a-rel.ko needs unknown symbol __fentry__
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-a-rel.ko needs unknown symbol printk
+depmod: WARNING: /lib/modules/4.4.4/kernel/mod-foo-a-rel.ko needs unknown symbol module_layout

testsuite/test-depmod.c +30 -0

index e249c61..a4c8ed7 100644
@@ -183,4 +183,34 @@ DEFINE_TEST(depmod_search_order_external_last,
 		},
 	});
 
+#define RELOCATED_CRC_ROOTFS TESTSUITE_ROOTFS "test-depmod/relocated-crc"
+static noreturn int relocated_crc(const struct test *t)
+{
+	const char *progname = ABS_TOP_BUILDDIR "/tools/depmod";
+	const char *const args[] = {
+		progname,
+		"-aeE",
+		"/boot/symvers",
+		NULL,
+	};
+
+	test_spawn_prog(progname, args);
+	exit(EXIT_FAILURE);
+}
+DEFINE_TEST(relocated_crc,
+	.description = "check if depmod understand both relocated and non-relocated crcs",
+	.print_outputs = true,
+	.config = {
+		[TC_UNAME_R] = "4.4.4",
+		[TC_ROOTFS] = RELOCATED_CRC_ROOTFS,
+	},
+	.output = {
+	        .err = RELOCATED_CRC_ROOTFS "/output.err",
+		.files = (const struct keyval[]) {
+			{ RELOCATED_CRC_ROOTFS "/lib/modules/4.4.4/correct-modules.dep",
+			  RELOCATED_CRC_ROOTFS "/lib/modules/4.4.4/modules.dep" },
+			{ }
+		},
+	});
+
 TESTSUITE_MAIN();

Signature · 9 lines

2.13.3





-- 
WBR,
Yauheni Kaliuta