Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 28 Jun 2018 14:59:25 +0200
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Cc: Vasily Averin <vvs@...tuozzo.com>
Subject: Re: 32-bit syscall breakage in -431 kernel with KAISER

On Tue, Jun 26, 2018 at 09:13:43PM +0200, Solar Designer wrote:
> per my review of the full struct tss_struct, the stack[] field
> offset is:
> 
> 4+8*5+4*2+2*2+1025*8+8 = 8264
> 
> So it's not 16-byte aligned.  But that's the start of stack[], and we
> need the stack pointer to be aligned.  Yet I suppose aligning stack[]
> itself is in fact the most appropriate fix.
> 
> Reviewing the diffs between e.g. -423 and -431, I see this stack[] in
> struct tss is in fact newly added:

At first glance, RHEL6 might also be affected.  From 2.6.32-754.el6 as
currently used by that OpenVZ branch:

struct x86_hw_tss {
        u32                     reserved1;
        u64                     sp0;
        u64                     sp1;
        u64                     sp2;
        u64                     reserved2;
        u64                     ist[7];
        u32                     reserved3;
        u32                     reserved4;
        u16                     reserved5;
        u16                     io_bitmap_base;

} __attribute__((packed)) ____cacheline_aligned;

This is 4+8*5+4*2+2*2 = 56 bytes like above.

struct tss_struct {
        /*
         * The hardware state:
         */
        struct x86_hw_tss       x86_tss;

        /*
         * The extra 1 is there because the CPU will access an
         * additional byte beyond the end of the IO permission
         * bitmap. The extra byte must be all 1 bits, and must
         * be within the limit.
         */
        unsigned long           io_bitmap[IO_BITMAP_LONGS + 1];

        /*
         * .. and then another 0x100 bytes for the emergency kernel stack:
         */
#ifndef __GENKSYMS__
        unsigned long           stack_canary;
        unsigned long           stack[256];
#else
        unsigned long           stack[64];
#endif

        /*
         *
         * The Intel SDM says (Volume 3, 7.2.1):
         *
         *  Avoid placing a page boundary in the part of the TSS that the
         *  processor reads during a task switch (the first 104 bytes). The
         *  processor may not correctly perform address translations if a
         *  boundary occurs in this area. During a task switch, the processor
         *  reads and writes into the first 104 bytes of each TSS (using
         *  contiguous physical addresses beginning with the physical address
         *  of the first byte of the TSS). So, after TSS access begins, if
         *  part of the 104 bytes is not physically contiguous, the processor
         *  will access incorrect information without generating a page-fault
         *  exception.
         *
         * There are also a lot of errata involving the TSS spanning a page
         * boundary.  Assert that we're not doing that.
         */
} __attribute__((__aligned__(PAGE_SIZE)));

This second struct lacks __attribute__((packed)), but since 56 is a
multiple of 8 and since everything after it appears to only require an
8-byte alignment as far as gcc is concerned, I'd expect stack[] to be
misaligned just like we think it is on RHEL5 - except if this is built
with __GENKSYMS__.  Is it?

In that kernel, the stack_canary check is inside "#ifdef
CONFIG_DEBUG_STACKOVERFLOW", so if that option isn't enabled then I
guess this kernel would build with __GENKSYMS__.

Alexander

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.