Re: [PATCH v6 3/6] fs/procfs: add build ID fetching to PROCMAP_QUERY API

[Andi Kleen <ak@linux.intel.com>]

Andrii Nakryiko <andrii@kernel.org> writes:

> The need to get ELF build ID reliably is an important aspect when
> dealing with profiling and stack trace symbolization, and
> /proc/<pid>/maps textual representation doesn't help with this.
>
> To get backing file's ELF build ID, application has to first resolve
> VMA, then use it's start/end address range to follow a special
> /proc/<pid>/map_files/<start>-<end> symlink to open the ELF file (this
> is necessary because backing file might have been removed from the disk
> or was already replaced with another binary in the same file path.
>
> Such approach, beyond just adding complexity of having to do a bunch of
> extra work, has extra security implications. Because application opens
> underlying ELF file and needs read access to its entire contents (as far
> as kernel is concerned), kernel puts additional capable() checks on
> following /proc/<pid>/map_files/<start>-<end> symlink. And that makes
> sense in general.

I was curious about this statement. It has still certainly potential
for side channels e.g. for files that are execute only, or with
some other special protection.

But actually just looking at the parsing code it seems to fail basic
TOCTTOU rules, and since you don't check if the VMA mapping is executable
(I think), so there's no EBUSY checking for writes, it likely is exploitable.


        /* only supports phdr that fits in one page */
                if (ehdr->e_phnum >
                   (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
                <---------- check in memory
                                return -EINVAL;

        phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));

<---- but page is shared in the page cache. So if anybody manages to map
it for write 


        for (i = 0; i < ehdr->e_phnum; ++i) {   <----- this loop can go
                        off into the next page.
                        if (phdr[i].p_type == PT_NOTE &&
                                            !parse_build_id(page_addr, build_id, size,
                                                            page_addr + phdr[i].p_offset,
                                                            phdr[i].p_filesz))
                                                                                    return 0;

Here's an untested patch


diff --git a/lib/buildid.c b/lib/buildid.c
index 7954dd92e36c..6c022fcd03ec 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -72,19 +72,20 @@ static int get_build_id_32(const void *page_addr, unsigned char *build_id,
 	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
 	Elf32_Phdr *phdr;
 	int i;
+	unsigned phnum = READ_ONCE(ehdr->e_phnum);
 
 	/* only supports phdr that fits in one page */
-	if (ehdr->e_phnum >
+	if (phnum >
 	    (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr))
 		return -EINVAL;
 
 	phdr = (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr));
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
+	for (i = 0; i < phnum; ++i) {
 		if (phdr[i].p_type == PT_NOTE &&
 		    !parse_build_id(page_addr, build_id, size,
 				    page_addr + phdr[i].p_offset,
-				    phdr[i].p_filesz))
+				    READ_ONCE(phdr[i].p_filesz)))
 			return 0;
 	}
 	return -EINVAL;
@@ -97,15 +98,16 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id,
 	Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
 	Elf64_Phdr *phdr;
 	int i;
+	unsigned phnum = READ_ONCE(ehdr->e_phnum);
 
 	/* only supports phdr that fits in one page */
-	if (ehdr->e_phnum >
+	if (phnum >
 	    (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr))
 		return -EINVAL;
 
 	phdr = (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr));
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
+	for (i = 0; i < phnum; ++i) {
 		if (phdr[i].p_type == PT_NOTE &&
 		    !parse_build_id(page_addr, build_id, size,
 				    page_addr + phdr[i].p_offset,