[3.5.y.z extended stable] Patch "ipc, msg: fix message length check for negative values" has been added to staging queue

[Mathias Krause]

On 19 November 2013 14:18, Luis Henriques <luis.henriques at canonical.com> wrote:
> This is a note to let you know that I have just added a patch titled
>
>     ipc, msg: fix message length check for negative values
>
> to the linux-3.5.y-queue branch of the 3.5.y.z extended stable tree
> which can be found at:
>
>  http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.5.y-queue
>
> If you, or anyone else, feels it should not be added to this tree, please
> reply to this email.
>
> For more information about the 3.5.y.z tree, see
> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
>
> Thanks.
> -Luis
>
> ------
>
> From dd8a5d84708799e2e7fb1fd291695764abd1fe05 Mon Sep 17 00:00:00 2001
> From: Mathias Krause <minipli at googlemail.com>
> Date: Tue, 12 Nov 2013 15:11:47 -0800
> Subject: ipc, msg: fix message length check for negative values
>
> commit 4e9b45a19241354daec281d7a785739829b52359 upstream.
>
> 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 at linux-foundation.org: fix i386 min() warnings]
> Signed-off-by: Mathias Krause <minipli at googlemail.com>
> Cc: Pax Team <pageexec at freemail.hu>
> Cc: Davidlohr Bueso <davidlohr at hp.com>
> Cc: Brad Spengler <spender at grsecurity.net>
> Cc: Manfred Spraul <manfred at colorfullife.com>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> [ luis: backported to 3.5:
>   - adjusted context
>   - dropped changes to alloc_msg() and copy_msg() ]

The whole point of the patch is to fix a bug induced by unintended
sign extension, so dropping that hunk nullifies the fix. Fortunately,
the sign extension bug doesn't trigger on your kernel as DATALEN_MSG
isn't signed, but unsigned. You should have noticed that already as
you had to adapt that hunk as well -- see below. ;)

> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
> ---
>  include/linux/msg.h |  6 +++---
>  ipc/msgutil.c       | 12 ++++++------
>  ipc/util.h          |  4 ++--
>  3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/msg.h b/include/linux/msg.h
> index 56abf155..70fc369 100644
> --- a/include/linux/msg.h
> +++ b/include/linux/msg.h
> @@ -76,9 +76,9 @@ struct msginfo {
>
>  /* one msg_msg structure for each message */
>  struct msg_msg {
> -       struct list_head m_list;
> -       long  m_type;
> -       int m_ts;           /* message text size */
> +       struct list_head m_list;
> +       long m_type;
> +       size_t m_ts;            /* message text size */
>         struct msg_msgseg* next;
>         void *security;
>         /* the actual message follows immediately */
> diff --git a/ipc/msgutil.c b/ipc/msgutil.c
> index 26143d3..52be05a 100644
> --- a/ipc/msgutil.c
> +++ b/ipc/msgutil.c
> @@ -39,15 +39,15 @@ struct msg_msgseg {
>         /* the next part of the message follows immediately */
>  };
>

> -#define DATALEN_MSG    (PAGE_SIZE-sizeof(struct msg_msg))
> -#define DATALEN_SEG    (PAGE_SIZE-sizeof(struct msg_msgseg))
> +#define DATALEN_MSG    ((size_t)PAGE_SIZE-sizeof(struct msg_msg))
> +#define DATALEN_SEG    ((size_t)PAGE_SIZE-sizeof(struct msg_msgseg))

This change is a no-op. Albeit, the former definition of DATALEN_MSG
makes the bug not to manifest itself. So unless you intend to backport
commit 3d8fa456d5 "ipc: clamp with min()" as well, just drop the
patch.

>
> -struct msg_msg *load_msg(const void __user *src, int len)
> +struct msg_msg *load_msg(const void __user *src, size_t len)
>  {
>         struct msg_msg *msg;
>         struct msg_msgseg **pseg;
>         int err;
> -       int alen;
> +       size_t alen;
>
>         alen = len;
>         if (alen > DATALEN_MSG)
> @@ -101,9 +101,9 @@ out_err:
>         return ERR_PTR(err);
>  }
>
> -int store_msg(void __user *dest, struct msg_msg *msg, int len)
> +int store_msg(void __user *dest, struct msg_msg *msg, size_t len)
>  {
> -       int alen;
> +       size_t alen;
>         struct msg_msgseg *seg;
>
>         alen = len;
> diff --git a/ipc/util.h b/ipc/util.h
> index 6f5c20b..0bfc934 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -138,8 +138,8 @@ int ipc_parse_version (int *cmd);
>  #endif
>
>  extern void free_msg(struct msg_msg *msg);
> -extern struct msg_msg *load_msg(const void __user *src, int len);
> -extern int store_msg(void __user *dest, struct msg_msg *msg, int len);
> +extern struct msg_msg *load_msg(const void __user *src, size_t len);
> +extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len);
>
>  extern void recompute_msgmni(struct ipc_namespace *);
>
> --
> 1.8.3.2
>

Regards,
Mathias