Bug 232289
| Summary: | kern/link_elf.c fails for small sections sizes (<sizeof(void *)) (also affects pcpu and vnet) | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Bjoern A. Zeeb <bz> |
| Component: | kern | Assignee: | Bjoern A. Zeeb <bz> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | bz, emaste |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
| Bug Depends on: | |||
| Bug Blocks: | 230857 | ||
A commit references this bug: Author: bz Date: Thu Oct 18 20:20:42 UTC 2018 New revision: 339431 URL: https://svnweb.freebsd.org/changeset/base/339431 Log: In r78161 the lookup_set linker method was introduced which optionally returns the section start and stop locations as well as a count if the caller asks for them. There was only one out-of-file consumer of count which did not actually use it and hence was eliminated in r339407. In r194784 parse_dpcpu(), and in r195699 parse_vnet() (a copy of the former) started to use the link_elf_lookup_set() interface internally also asking for the count. count is computed as the difference of the void **stop - void **start locations and as such, if the absoulte numbers (stop - start) % sizeof(void *) != 0 a round-down happens, e.g., **stop 0x1003 - **start 0x1000 => count 0. To get the section size instead of "count is the number of pointer elements in the section", the parse_*() functions do a count *= sizeof(void *). They use the result to allocate memory and copy the section data into the "master" and per-instance memory regions with a size of count. As a result of count possibly round-down this can miss the last bytes of the section. The good news is that we do not touch out of bounds memory during these operations (we may at a later stage if the last bytes would overflow the master sections). Given relocation in elf_relocaddr() works based on the absolute numbers of start and stop, this means that we can possibly try to access relocated data which was never copied and hence we get random garbage or at best zeroed memory. Stop the two (last) consumers of count (the parse_*() functions) from using count as well, and calculate the section size based on the absolute numbers of stop and start and use the proper size for the memory allocation and data copies. This will make the symbols in the last bytes of the pcpu or vnet sections be presented as expected. PR: 232289 Approved by: re (gjb) MFC after: 2 weeks Changes: head/sys/kern/link_elf.c X-MFS with r339407 A commit references this bug: Author: bz Date: Fri Nov 2 14:13:32 UTC 2018 New revision: 340053 URL: https://svnweb.freebsd.org/changeset/base/340053 Log: MFC r339431: In r78161 the lookup_set linker method was introduced which optionally returns the section start and stop locations as well as a count if the caller asks for them. There was only one out-of-file consumer of count which did not actually use it and hence was eliminated in r339407. In r194784 parse_dpcpu(), and in r195699 parse_vnet() (a copy of the former) started to use the link_elf_lookup_set() interface internally also asking for the count. count is computed as the difference of the void **stop - void **start locations and as such, if the absoulte numbers (stop - start) % sizeof(void *) != 0 a round-down happens, e.g., **stop 0x1003 - **start 0x1000 => count 0. To get the section size instead of "count is the number of pointer elements in the section", the parse_*() functions do a count *= sizeof(void *). They use the result to allocate memory and copy the section data into the "master" and per-instance memory regions with a size of count. As a result of count possibly round-down this can miss the last bytes of the section. The good news is that we do not touch out of bounds memory during these operations (we may at a later stage if the last bytes would overflow the master sections). Given relocation in elf_relocaddr() works based on the absolute numbers of start and stop, this means that we can possibly try to access relocated data which was never copied and hence we get random garbage or at best zeroed memory. Stop the two (last) consumers of count (the parse_*() functions) from using count as well, and calculate the section size based on the absolute numbers of stop and start and use the proper size for the memory allocation and data copies. This will make the symbols in the last bytes of the pcpu or vnet sections be presented as expected. PR: 232289 Changes: _U stable/11/ stable/11/sys/kern/link_elf.c |
Hi, debugging PR230857 I noticed a small problem (here's output from instrumented kernel) loading a module with a single char variable in a vnet_set and the . = . + 1 workaround currently in place (hence 2 bytes section size). The problem here is count == 0: XXX-BZ link_elf_lookup_set: count 0 stop 0x1d400000 start 0x1d400000 <<== pcpu (unused) XXX-BZ link_elf_lookup_set: count 0 stop 0x1d401272 start 0x1d401270 <<== vnet XXX-BZ parse_vnet: count 0 start 0x1d401270 stop 0x1d401272 XXX-BZ parse_vnet:682 error = 0 XXX-BZ link_elf_reloc_local:1619 /boot/kernel/z1.ko addr 0x1d400000 rel 0 relsize 0 rela 0 relasize 0 From elfdump -a /boot/kernel/z1.ko: entry: 6 sh_name: set_vnet sh_type: SHT_PROGBITS sh_flags: SHF_WRITE|SHF_ALLOC sh_addr: 0x1270 sh_offset: 624 sh_size: 2 sh_link: 0 sh_info: 0 sh_addralign: 1 sh_entsize: 0 entry: 1 st_name: __stop_set_vnet st_value: 0x1272 st_size: 0 st_info: STT_NOTYPE STB_GLOBAL st_shndx: 65521 entry: 3 st_name: ___set_vnet_pad st_value: 0x1 st_size: 0 st_info: STT_NOTYPE STB_GLOBAL st_shndx: 65521 entry: 9 st_name: __start_set_vnet st_value: 0x1270 st_size: 0 st_info: STT_NOTYPE STB_GLOBAL st_shndx: 65521 entry: 1 st_name: vnet_entry_achar st_value: 0x1270 st_size: 1 st_info: STT_OBJECT STB_LOCAL st_shndx: 6 sys/kern/link_elf.c: link_elf_lookup_set() does read the start and stop symbols: void **start, **stop; int len, error = 0, count; .. /* and the number of entries */ count = stop - start; printf("XXX-BZ %s: count %d stop %#jx start %#jx\n", __func__, count, (uintmax_t)(uintptr_t)stop, (uintmax_t)(uintptr_t)start); The problem is that the addresses are stored in void ** and so 2 - 0 ! = 2 but 0. Hence, with the count being 0, so when parse_dpcpu() and parse_vnet() do a count *= sizeof(void *); 0 * x will stay 0. So the extra memory allocated in the set handlers will be roundup2(size, sizeof(void *)) or 0. Also the copy functions later will copy 0 bytes. While there is no error, neither the memory needed is allocated, nor the data needed is copied into the memory area. With the start/stop symbols stored properly, future relocations would work, the memory accessed be bogus however.