| Message ID | 1384867110-23787-1-git-send-email-luis.henriques@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 19 November 2013 14:18, Luis Henriques <luis.henriques@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@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@linux-foundation.org: fix i386 min() warnings] > Signed-off-by: Mathias 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> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@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@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
On Wed, Nov 20, 2013 at 10:17:51AM +0100, Mathias Krause wrote: > > -#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. > Thanks you for your review. Since 3d8fa456d5 hasn't been tagged for stable, I will just drop this patch from the 3.5 queue. Cheers, -- Luis
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)) -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 *);
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@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@linux-foundation.org: fix i386 min() warnings] Signed-off-by: Mathias 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> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> [ luis: backported to 3.5: - adjusted context - dropped changes to alloc_msg() and copy_msg() ] Signed-off-by: Luis Henriques <luis.henriques@canonical.com> --- include/linux/msg.h | 6 +++--- ipc/msgutil.c | 12 ++++++------ ipc/util.h | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) -- 1.8.3.2