Bug 232289 - kern/link_elf.c fails for small sections sizes (<sizeof(void *)) (also affects pcpu and vnet)
Summary: kern/link_elf.c fails for small sections sizes (<sizeof(void *)) (also affec...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Bjoern A. Zeeb
URL:
Keywords:
Depends on:
Blocks: 230857
  Show dependency treegraph
 
Reported: 2018-10-15 15:17 UTC by Bjoern A. Zeeb
Modified: 2018-11-02 14:16 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bjoern A. Zeeb freebsd_committer 2018-10-15 15:17:57 UTC
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.
Comment 1 commit-hook freebsd_committer 2018-10-18 20:20:58 UTC
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
Comment 2 Bjoern A. Zeeb freebsd_committer 2018-10-18 21:35:28 UTC
X-MFS with r339407
Comment 3 commit-hook freebsd_committer 2018-11-02 14:13:58 UTC
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