Re: [PATCHSET] saner elf compat

[hpa]

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.