Skip to contentSkip to conversation

libkmod: check size in elf_get_mem (Take 2)

PATCH In review Tobias Stoeckmann 2 participants 2 messages Feb 11, 2015 – Feb 17, 2015

lore
Share & tools
lore

Tobias Stoeckmann

author
#0 lore raw

PATCH 1/1 libkmod: check size in elf_get_mem (Take 2)

Revision 2 of this patch, fixes wrong boundary checks of previous attempt. Can definitely need some review here. :)

The function elf_get_mem properly checks if offset is smaller than elf->size, but does not perform further memory size validations.

Supply desired size as third argument to elf_get_mem, which is in sync with elf_get_uint/elf_set_uint then.

As you can see, further unbounded string operations like strlen are still prone to lead to out-of-bounds access, which will be covered in another patch.

---
 libkmod/libkmod-elf.c | 54 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 19 deletions(-)
libkmod/libkmod-elf.c +35 -19
index 2f50ad2..ebd3dea 100644
@@ -197,12 +197,15 @@ static inline int elf_set_uint(struct kmod_elf *elf, uint64_t offset, uint64_t s
 	return 0;
 }
 
-static inline const void *elf_get_mem(const struct kmod_elf *elf, uint64_t offset)
+static inline const void *elf_get_mem(const struct kmod_elf *elf, uint64_t offset, uint64_t size)
 {
-	assert(offset < elf->size);
-	if (offset >= elf->size) {
-		ELFDBG(elf, "out-of-bounds: %"PRIu64" >= %"PRIu64" (ELF size)\n",
-		       offset, elf->size);
+	uint64_t end;
+
+	assert(!addu64_overflow(offset, size, &end));
+	assert(end <= elf->size);
+	if (addu64_overflow(offset, size, &end) || end > elf->size) {
+		ELFDBG(elf, "out-of-bounds: %"PRIu64" + %"PRIu64" > %"PRIu64" (ELF size)\n",
+		       offset, size, elf->size);
 		return NULL;
 	}
 	return elf->memory + offset;

@@ -218,7 +221,8 @@ static inline const void *elf_get_section_header(const struct kmod_elf *elf, uin
 		return NULL;
 	}
 	return elf_get_mem(elf, elf->header.section.offset +
-			   idx * elf->header.section.entry_size);
+			   idx * elf->header.section.entry_size,
+			   elf->header.section.entry_size);
 }
 
 static inline int elf_get_section_info(const struct kmod_elf *elf, uint16_t idx, uint64_t *offset, uint64_t *size, uint32_t *nameoff)

@@ -266,7 +270,8 @@ static inline int elf_get_section_info(const struct kmod_elf *elf, uint16_t idx,
 static const char *elf_get_strings_section(const struct kmod_elf *elf, uint64_t *size)
 {
 	*size = elf->header.strings.size;
-	return elf_get_mem(elf, elf->header.strings.offset);
+	return elf_get_mem(elf, elf->header.strings.offset,
+			   elf->header.strings.size);
 }
 
 struct kmod_elf *kmod_elf_new(const void *memory, off_t size)

@@ -307,11 +312,13 @@ struct kmod_elf *kmod_elf_new(const void *memory, off_t size)
 	elf->header.strings.section = READV(e_shstrndx);	\
 	elf->header.machine = READV(e_machine)
 	if (elf->class & KMOD_ELF_32) {
-		const Elf32_Ehdr *hdr _unused_ = elf_get_mem(elf, 0);
+		size_t hdr_size = sizeof(Elf32_Ehdr);
+		const Elf32_Ehdr *hdr _unused_ = elf_get_mem(elf, 0, hdr_size);
 		LOAD_HEADER;
 		shdr_size = sizeof(Elf32_Shdr);
 	} else {
-		const Elf64_Ehdr *hdr _unused_ = elf_get_mem(elf, 0);
+		size_t hdr_size = sizeof(Elf64_Ehdr);
+		const Elf64_Ehdr *hdr _unused_ = elf_get_mem(elf, 0, hdr_size);
 		LOAD_HEADER;
 		shdr_size = sizeof(Elf64_Shdr);
 	}

@@ -417,7 +424,7 @@ int kmod_elf_get_section(const struct kmod_elf *elf, const char *section, const
 		if (!streq(section, n))
 			continue;
 
-		*buf = elf_get_mem(elf, off);
+		*buf = elf_get_mem(elf, off, size);
 		*buf_size = size;
 		return 0;
 	}

@@ -534,7 +541,8 @@ int kmod_elf_get_modversions(const struct kmod_elf *elf, struct kmod_modversion
 	slen = 0;
 
 	for (i = 0; i < count; i++, off += MODVERSION_SEC_SIZE) {
-		const char *symbol = elf_get_mem(elf, off + offcrc);
+		const char *symbol = elf_get_mem(elf, off + offcrc,
+						 MODVERSION_SEC_SIZE - offcrc);
 
 		if (symbol[0] == '.')
 			symbol++;

@@ -551,7 +559,8 @@ int kmod_elf_get_modversions(const struct kmod_elf *elf, struct kmod_modversion
 
 	for (i = 0; i < count; i++, off += MODVERSION_SEC_SIZE) {
 		uint64_t crc = elf_get_uint(elf, off, offcrc);
-		const char *symbol = elf_get_mem(elf, off + offcrc);
+		const char *symbol = elf_get_mem(elf, off + offcrc,
+						 MODVERSION_SEC_SIZE - offcrc);
 		size_t symbollen;
 
 		if (symbol[0] == '.')

@@ -806,7 +815,8 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
 			goto fallback;
 		}
 
-		name = elf_get_mem(elf, str_off + name_off);
+		name = elf_get_mem(elf, str_off + name_off,
+				   strtablen - name_off);
 
 		if (strncmp(name, crc_str, crc_strlen) != 0)
 			continue;

@@ -846,7 +856,8 @@ int kmod_elf_get_symbols(const struct kmod_elf *elf, struct kmod_modversion **ar
 			info = READV(st_info);
 		}
 #undef READV
-		name = elf_get_mem(elf, str_off + name_off);
+		name = elf_get_mem(elf, str_off + name_off,
+				   strtablen - name_off);
 		if (strncmp(name, crc_str, crc_strlen) != 0)
 			continue;
 		name += crc_strlen;

@@ -889,7 +900,8 @@ static int kmod_elf_crc_find(const struct kmod_elf *elf, const void *versions, u
 
 	off = (const uint8_t *)versions - elf->memory;
 	for (i = 0; i < versionslen; i += verlen) {
-		const char *symbol = elf_get_mem(elf, off + i + crclen);
+		const char *symbol = elf_get_mem(elf, off + i + crclen,
+						 verlen - crclen);
 		if (!streq(name, symbol))
 			continue;
 		*crc = elf_get_uint(elf, off + i, crclen);

@@ -1038,7 +1050,8 @@ int kmod_elf_get_dependency_symbols(const struct kmod_elf *elf, struct kmod_modv
 			return -EINVAL;
 		}
 
-		name = elf_get_mem(elf, str_off + name_off);
+		name = elf_get_mem(elf, str_off + name_off,
+				   strtablen - name_off);
 		if (name[0] == '\0') {
 			ELFDBG(elf, "empty symbol name at index %"PRIu64"\n", i);
 			continue;

@@ -1059,7 +1072,8 @@ int kmod_elf_get_dependency_symbols(const struct kmod_elf *elf, struct kmod_modv
 		for (i = 0; i < vercount; i++) {
 			if (visited_versions[i] == 0) {
 				const char *name;
-				name = elf_get_mem(elf, ver_off + i * verlen + crclen);
+				name = elf_get_mem(elf, ver_off + i * verlen + crclen,
+						   verlen - crclen);
 				slen += strlen(name) + 1;
 
 				count++;

@@ -1126,7 +1140,8 @@ int kmod_elf_get_dependency_symbols(const struct kmod_elf *elf, struct kmod_modv
 				continue;
 		}
 
-		name = elf_get_mem(elf, str_off + name_off);
+		name = elf_get_mem(elf, str_off + name_off,
+				   strtablen - name_off);
 		if (name[0] == '\0') {
 			ELFDBG(elf, "empty symbol name at index %"PRIu64"\n", i);
 			continue;

@@ -1168,7 +1183,8 @@ int kmod_elf_get_dependency_symbols(const struct kmod_elf *elf, struct kmod_modv
 		if (visited_versions[i] != 0)
 			continue;
 
-		name = elf_get_mem(elf, ver_off + i * verlen + crclen);
+		name = elf_get_mem(elf, ver_off + i * verlen + crclen,
+				   verlen - crclen);
 		slen = strlen(name);
 		crc = elf_get_uint(elf, ver_off + i * verlen, crclen);
Signature · 1 line
2.3.0
#1 lore raw
Quoted 2 lines · Tobias Stoeckmann

Revision 2 of this patch, fixes wrong boundary checks of previous attempt. Can definitely need some review here. :)

Since this message is going directly to the commit, please put sentences like above after the "---" in your email. This way "git am" works and I don't have to manually edit the commit message. Also there's no need to "sign" the email.

Quoted 20 lines

The function elf_get_mem properly checks if offset is smaller than elf->size, but does not perform further memory size validations.

Supply desired size as third argument to elf_get_mem, which is in sync with elf_get_uint/elf_set_uint then.

As you can see, further unbounded string operations like strlen are still prone to lead to out-of-bounds access, which will be covered in another patch.

---
 libkmod/libkmod-elf.c | 54 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 19 deletions(-)
libkmod/libkmod-elf.c +0 -0
index 2f50ad2..ebd3dea 100644
@@ -197,12 +197,15 @@ static inline int elf_set_uint(struct kmod_elf *elf, uint64_t offset, uint64_t s
        return 0;
 }

libkmod/libkmod-elf.c static inline int elf_set_uint(struct kmod_elf *elf, uint64_t offset, uint64_t s · line 204

Quoted 10 lines
libkmod/libkmod-elf.c +4 -5
-static inline const void *elf_get_mem(const struct kmod_elf *elf, uint64_t offset)
+static inline const void *elf_get_mem(const struct kmod_elf *elf, uint64_t offset, uint64_t size)
 {
-       assert(offset < elf->size);
-       if (offset >= elf->size) {
-               ELFDBG(elf, "out-of-bounds: %"PRIu64" >= %"PRIu64" (ELF size)\n",
-                      offset, elf->size);
+       uint64_t end;
+
+       assert(!addu64_overflow(offset, size, &end));

assert may be defined to nothing if "NDEBUG" is define at compile time. So don't put this check with side-effects inside the assert(). If the check would be always on (which is what we want here) we need to replace the assert with proper "if (!addu64...() || end > elf->size)"

the rest looks good.

Signature · 1 line
Lucas De Marchi