On December 5, 2020 7:23:05 PM PST, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: >On Thu, Dec 03, 2020 at 11:03:36PM +0000, Al Viro wrote: >> > > The answer (for mainline) is that mips compat does *NOT* want >> > > COMPAT_BINFMT_ELF. Not a problem with that series, though, so >I'd >> > > retested it (seems to work, both for x86_64 and mips64, execs and >> > > coredumps for all ABIs alike), with centralization of Kconfig >logics >> > > thrown in. >> > >> > Well, the diffstat looks nice: >> > >> > > 26 files changed, 127 insertions(+), 317 deletions(-) >> > >> > and the patches didn't trigger anything for me, but how much did >this >> > get tested? Do you actually have both kinds of 32-bit elf mips >> > binaries around and a machine to test on? >> >> Yes (aptitude install gcc-multilib on debian mips64el/stretch sets >the toolchain >> and libraries just fine, and then it's just a matter of -mabi=n32 >passed >> to gcc). "Machine" is qemu-system-mips64el -machine malta -m 1024 >-cpu 5KEc >> and the things appear to work; I hadn't tried that on the actual >hardware. >> I do have a Loongson-2 box, but it would take a while to dig it out >and >> get it up-to-date. >> >> > Linux-mips was cc'd, but I'm adding Thomas B to the cc here >explicitly >> > just so that he has a heads-up on this thing and can go and look at >> > the mailing list in case it goes to a separate mailbox for him.. >> >> I would certainly appreciate review and testing - this branch sat >> around in the "should post it someday" state since June (it was >> one of the followups grown from regset work back then), and I'm >> _not_ going to ask pulling it without an explicit OK from mips >> folks. > >BTW, there's something curious going on in ELF binary recognition for >x32. Unlike other 64bit architectures, here we have a 32bit binary >that successfully passes the native elf_check_arch(). Usually we >either have different EM_... values for 64bit and 32bit (e.g. for ppc >and sparc) or we have an explicit check for ->e_ident[EI_CLASS] >having the right value (ELFCLASS32 or ELFCLASS64 for 32bit and 64bit >binaries resp.) > >For x32 that's not true - we use EM_X86_64 for ->e_machine and that's >the only thing the native elf_check_arch() is looking at. IOW, >it looks like amd64 elf_load_binary() progresses past elf_check_arch() >for x32 binaries. And gets to load_elf_phdrs(), which would appear >to have a check of its own that should reject the sucker: > /* > * If the size of this structure has changed, then punt, since > * we will be doing the wrong thing. > */ > if (elf_ex->e_phentsize != sizeof(struct elf_phdr)) > goto out; >After all, ->e_phentsize is going to be 32 (sizeof(struct elf32_phdr) >rather than expected 56 (sizeof(struct elf64_phdr)) and off we bugger, >even though it happens at slightly later point than usual. Except that >we are looking at struct elf64_hdr ->e_phentsize - in struct elf32_hdr. >I.e. at offset 54, two bytes past the end of in-file struct elf32_hdr. > >Usually we won't find 0x38 0x00 in that location, so everything works, >but IMO that's too convoluted. > >Peter, is there any reason not to check ->ei_ident[EI_CLASS] in >amd64 elf_check_arch()? It's a 1-byte load from hot cacheline >(offset 4 and we'd just read the 4 bytes at offsets 0..3) + >compare + branch not taken, so performance impact is pretty much >nil. I'm not saying it's a security problem or anything of that >sort, just that it makes the analysis more subtle than it ought >to be... > >Is it about some malformed homegrown 64bit binaries with BS value >at offset 4? Confused... I can't think of any. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
