• Mathias Krause's avatar
    ipc, msg: fix message length check for negative values · 4e9b45a1
    Mathias Krause authored
    On 64 bit systems the test for negative message sizes is bogus as the
    size, which may be positive when evaluated as a long, will get truncated
    to an int when passed to load_msg().  So a long might very well contain a
    positive value but when truncated to an int it would become negative.
    
    That in combination with a small negative value of msg_ctlmax (which will
    be promoted to an unsigned type for the comparison against msgsz, making
    it a big positive value and therefore make it pass the check) will lead to
    two problems: 1/ The kmalloc() call in alloc_msg() will allocate a too
    small buffer as the addition of alen is effectively a subtraction.  2/ The
    copy_from_user() call in load_msg() will first overflow the buffer with
    userland data and then, when the userland access generates an access
    violation, the fixup handler copy_user_handle_tail() will try to fill the
    remainder with zeros -- roughly 4GB.  That almost instantly results in a
    system crash or reset.
    
      ,-[ Reproducer (needs to be run as root) ]--
      | #include <sys/stat.h>
      | #include <sys/msg.h>
      | #include <unistd.h>
      | #include <fcntl.h>
      |
      | int main(void) {
      |     long msg = 1;
      |     int fd;
      |
      |     fd = open("/proc/sys/kernel/msgmax", O_WRONLY);
      |     write(fd, "-1", 2);
      |     close(fd);
      |
      |     msgsnd(0, &msg, 0xfffffff0, IPC_NOWAIT);
      |
      |     return 0;
      | }
      '---
    
    Fix the issue by preventing msgsz from getting truncated by consistently
    using size_t for the message length.  This way the size checks in
    do_msgsnd() could still be passed with a negative value for msg_ctlmax but
    we would fail on the buffer allocation in that case and error out.
    
    Also change the type of m_ts from int to size_t to avoid similar nastiness
    in other code paths -- it is used in similar constructs, i.e.  signed vs.
    unsigned checks.  It should never become negative under normal
    circumstances, though.
    
    Setting msg_ctlmax to a negative value is an odd configuration and should
    be prevented.  As that might break existing userland, it will be handled
    in a separate commit so it could easily be reverted and reworked without
    reintroducing the above described bug.
    
    Hardening mechanisms for user copy operations would have catched that bug
    early -- e.g.  checking slab object sizes on user copy operations as the
    usercopy feature of the PaX patch does.  Or, for that matter, detect the
    long vs.  int sign change due to truncation, as the size overflow plugin
    of the very same patch does.
    
    [akpm@linux-foundation.org: fix i386 min() warnings]
    Signed-off-by: default avatarMathias Krause <minipli@googlemail.com>
    Cc: Pax Team <pageexec@freemail.hu>
    Cc: Davidlohr Bueso <davidlohr@hp.com>
    Cc: Brad Spengler <spender@grsecurity.net>
    Cc: Manfred Spraul <manfred@colorfullife.com>
    Cc: <stable@vger.kernel.org>	[ v2.3.27+ -- yes, that old ;) ]
    Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
    Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
    4e9b45a1
util.h 5.83 KB