[PATCH 0/2] IPC DoS fix

[PATCH 0/2] IPC DoS fix

[Mathias Krause]

Hi Linus,

this series fixes a bug in the IPC code that allows root to instantly crash
the system. The first patch fixes the bug, the second one prevents msgmax
from getting negative in the first place. As that *might* break existing
userspace (for good?!) it's a separate patch.

The cc list is quite randomly choosen as there seem to be no maintainer for
ipc/. :/

Please apply!


Mathias Krause (2):
  ipc, msg: fix message length check for negative values
  ipc, msg: forbid negative values for "msgmax"

 ipc/ipc_sysctl.c |    6 +++---
 ipc/msg.c        |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] ipc, msg: fix message length check for negative values

[Mathias Krause]

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 checking for negative values the way they're used by
casting msgsz to int.

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.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pax Team <pageexec@freemail.hu>
Cc: <stable@vger.kernel.org>	# v2.3.27+ -- yes, that old ;)
---
 ipc/msg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 558aa91..d139b1e 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -667,7 +667,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
 
 	ns = current->nsproxy->ipc_ns;
 
-	if (msgsz > ns->msg_ctlmax || (long) msgsz < 0 || msqid < 0)
+	if (msgsz > ns->msg_ctlmax || (int) msgsz < 0 || msqid < 0)
 		return -EINVAL;
 	if (mtype < 1)
 		return -EINVAL;
-- 
1.7.10.4

[PATCH 2/2] ipc, msg: forbid negative values for "msgmax"

[Mathias Krause]

Negative message lengths make no sense, prevent them from being set.
They do more harm than gain.

In case a user wants to have "unlimited" message sizes she should
just use INT_MAX instead.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 ipc/ipc_sysctl.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 130dfec..76823b8 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -158,9 +158,7 @@ static int proc_ipcauto_dointvec_minmax(ctl_table *table, int write,
 
 static int zero;
 static int one = 1;
-#ifdef CONFIG_CHECKPOINT_RESTORE
 static int int_max = INT_MAX;
-#endif
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -198,7 +196,9 @@ static struct ctl_table ipc_kern_table[] = {
 		.data		= &init_ipc_ns.msg_ctlmax,
 		.maxlen		= sizeof (init_ipc_ns.msg_ctlmax),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &int_max,
 	},
 	{
 		.procname	= "msgmni",
-- 
1.7.10.4

Re: [PATCH 1/2] ipc, msg: fix message length check for negative values

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Sat, Nov 2, 2013 at 2:26 PM, Mathias Krause <minipli@googlemail.com> wrote:
> 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().

Quite frankly, wouldn't it be much nicer to just fix "load_msg()" instead?

Using "size_t" also gets rid of the games we play with DATALEN_MSG/SEG.

IOW, something like the attached..

Of course, we *also* should fix ns->msg_ctlmax to make clear you can't
use negative numbers there. No question about that. I think it would
be better to even avoid INT_MAX, because there are memory use concerns
and CPU usage ones too (we generate that list of
smaller-than-page-size fragments).

Hmm?

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2585 bytes --]

 include/linux/ipc_namespace.h |  6 +++---
 ipc/msgutil.c                 | 14 +++++++-------
 ipc/util.h                    |  4 ++--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 19c19a5eee29..f6c82de12541 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -34,9 +34,9 @@ struct ipc_namespace {
 	int		sem_ctls[4];
 	int		used_sems;
 
-	int		msg_ctlmax;
-	int		msg_ctlmnb;
-	int		msg_ctlmni;
+	unsigned int	msg_ctlmax;
+	unsigned int	msg_ctlmnb;
+	unsigned int	msg_ctlmni;
 	atomic_t	msg_bytes;
 	atomic_t	msg_hdrs;
 	int		auto_msgmni;
diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 491e71f2a1b8..3cd9cb959c74 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -41,15 +41,15 @@ struct msg_msgseg {
 	/* the next part of the message follows immediately */
 };
 
-#define DATALEN_MSG	(int)(PAGE_SIZE-sizeof(struct msg_msg))
-#define DATALEN_SEG	(int)(PAGE_SIZE-sizeof(struct msg_msgseg))
+#define DATALEN_MSG	(PAGE_SIZE-sizeof(struct msg_msg))
+#define DATALEN_SEG	(PAGE_SIZE-sizeof(struct msg_msgseg))
 
 
-static struct msg_msg *alloc_msg(int len)
+static struct msg_msg *alloc_msg(size_t len)
 {
 	struct msg_msg *msg;
 	struct msg_msgseg **pseg;
-	int alen;
+	size_t alen;
 
 	alen = min(len, DATALEN_MSG);
 	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
@@ -80,7 +80,7 @@ out_err:
 	return NULL;
 }
 
-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 *seg;
@@ -147,9 +147,9 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 	return ERR_PTR(-ENOSYS);
 }
 #endif
-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 = min(len, DATALEN_MSG);
diff --git a/ipc/util.h b/ipc/util.h
index f2f5036f2eed..59d78aa94987 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -148,9 +148,9 @@ 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 struct msg_msg *load_msg(const void __user *src, size_t len);
 extern struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst);
-extern int store_msg(void __user *dest, struct msg_msg *msg, int len);
+extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len);
 
 extern void recompute_msgmni(struct ipc_namespace *);
 

Re: [PATCH 1/2] ipc, msg: fix message length check for negative values

[Mathias Krause]

On 3 November 2013 01:35, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sat, Nov 2, 2013 at 2:26 PM, Mathias Krause <minipli@googlemail.com> wrote:
>> 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().
>
> Quite frankly, wouldn't it be much nicer to just fix "load_msg()" instead?

I thought of that too, but declined it as I thought it would require
much more changes then my simple patch does. As we cannot change the
underlying structures for msgctl(MSG_INFO/IPC_INFO), msg_ctlmax still
needs to fit into an int. But, for sure, we can still limit the user
visible range albeit handling msg_ctlmax internally as an unsigned
type.

> Using "size_t" also gets rid of the games we play with DATALEN_MSG/SEG.
>
> IOW, something like the attached..

Looking at your patch, it looks like my concerns are invalid. I'll
check it out and do a respin of the series later the day.

> Of course, we *also* should fix ns->msg_ctlmax to make clear you can't
> use negative numbers there. No question about that.

Agreed.

> I think it would
> be better to even avoid INT_MAX, because there are memory use concerns
> and CPU usage ones too (we generate that list of
> smaller-than-page-size fragments).

I'm uncertain about that one as changing the sysctl requires having
UID 0. So one needs to be privileged to be able to trigger the OOM
situation. Also, there might be userland out there trying to send big
messages -- I don't know. Are we willing to break them? What would be
a sensible maximum value for msgmax -- 16 MiB (8 KiB is the current
default)? Also, should we limit msgmnb and msgmni too then? Otherwise
one could just create multiple queues (in multiple namespaces) and
queue multiple messages to park a lot of memory this way. Or, one just
ignores this OOM situation and relies on the fact the root won't raise
the sysctls to insane values. What do you think?


Mathias

[PATCHv2 0/2] IPC DoS fix

[Mathias Krause]

Hi Linus,

version 2 of this series uses your approach to fix the issues by changing
load_msg() and friends to use a size_t for the message length. It differs
slightly from your patch to cover a few more places where the message
length is evaluated in sign extension problematic expressions. Also the
sysctl change is still a separate patch to allow reverting it in case it
breaks existing userland. It now handles all three sysctls: msgmax, msgmnb
and msgmni. All still capped at INT_MAX, though. They're privileged
sysctls after all. And setting them to INT_MAX does not end up in a system
crash, as it is now for negative values, but in an OOM killer invocation
instead which can be handled gracefully.

Regards,
Mathias


Mathias Krause (2):
  ipc, msg: fix message length check for negative values
  ipc, msg: forbid negative values for "msg{max,mnb,mni}"

 include/linux/ipc_namespace.h |    6 +++---
 include/linux/msg.h           |    6 +++---
 ipc/ipc_sysctl.c              |   20 ++++++++++++--------
 ipc/msgutil.c                 |   20 ++++++++++----------
 ipc/util.h                    |    4 ++--
 5 files changed, 30 insertions(+), 26 deletions(-)

-- 
1.7.10.4

[PATCHv2 1/2] ipc, msg: fix message length check for negative values

[Mathias Krause]

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.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pax Team <pageexec@freemail.hu>
Cc: stable@vger.kernel.org	# v2.3.27+ -- yes, that old ;)
---
v2:
 - pass size_t to all users instead of checking for the truncation case,
   as per Linus

 include/linux/msg.h |    6 +++---
 ipc/msgutil.c       |   20 ++++++++++----------
 ipc/util.h          |    4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/msg.h b/include/linux/msg.h
index 391af8d..e21f9d4 100644
--- a/include/linux/msg.h
+++ b/include/linux/msg.h
@@ -6,9 +6,9 @@
 
 /* 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 491e71f..4f840b3 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -41,15 +41,15 @@ struct msg_msgseg {
 	/* the next part of the message follows immediately */
 };
 
-#define DATALEN_MSG	(int)(PAGE_SIZE-sizeof(struct msg_msg))
-#define DATALEN_SEG	(int)(PAGE_SIZE-sizeof(struct msg_msgseg))
+#define DATALEN_MSG	(PAGE_SIZE-sizeof(struct msg_msg))
+#define DATALEN_SEG	(PAGE_SIZE-sizeof(struct msg_msgseg))
 
 
-static struct msg_msg *alloc_msg(int len)
+static struct msg_msg *alloc_msg(size_t len)
 {
 	struct msg_msg *msg;
 	struct msg_msgseg **pseg;
-	int alen;
+	size_t alen;
 
 	alen = min(len, DATALEN_MSG);
 	msg = kmalloc(sizeof(*msg) + alen, GFP_KERNEL);
@@ -80,12 +80,12 @@ out_err:
 	return NULL;
 }
 
-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 *seg;
 	int err = -EFAULT;
-	int alen;
+	size_t alen;
 
 	msg = alloc_msg(len);
 	if (msg == NULL)
@@ -117,8 +117,8 @@ out_err:
 struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 {
 	struct msg_msgseg *dst_pseg, *src_pseg;
-	int len = src->m_ts;
-	int alen;
+	size_t len = src->m_ts;
+	size_t alen;
 
 	BUG_ON(dst == NULL);
 	if (src->m_ts > dst->m_ts)
@@ -147,9 +147,9 @@ struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst)
 	return ERR_PTR(-ENOSYS);
 }
 #endif
-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 = min(len, DATALEN_MSG);
diff --git a/ipc/util.h b/ipc/util.h
index f2f5036..59d78aa 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -148,9 +148,9 @@ 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 struct msg_msg *load_msg(const void __user *src, size_t len);
 extern struct msg_msg *copy_msg(struct msg_msg *src, struct msg_msg *dst);
-extern int store_msg(void __user *dest, struct msg_msg *msg, int len);
+extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len);
 
 extern void recompute_msgmni(struct ipc_namespace *);
 
-- 
1.7.10.4

[PATCHv2 2/2] ipc, msg: forbid negative values for "msg{max,mnb,mni}"

[Mathias Krause]

Negative message lengths make no sense -- so don't do negative queue
lenghts or identifier counts. Prevent them from getting negative.

Also change the underlying data types to be unsigned to avoid harry
surprises with sign extensions in cases where those variables get
evaluated in unsigned expressions with bigger data types, e.g size_t.

In case a user still wants to have "unlimited" sizes she could just use
INT_MAX instead.

Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
v2:
 - handle msgmnb and msgmni, too
 - make the underlying variables unsigned, as per Linus

 include/linux/ipc_namespace.h |    6 +++---
 ipc/ipc_sysctl.c              |   20 ++++++++++++--------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 19c19a5..f6c82de 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -34,9 +34,9 @@ struct ipc_namespace {
 	int		sem_ctls[4];
 	int		used_sems;
 
-	int		msg_ctlmax;
-	int		msg_ctlmnb;
-	int		msg_ctlmni;
+	unsigned int	msg_ctlmax;
+	unsigned int	msg_ctlmnb;
+	unsigned int	msg_ctlmni;
 	atomic_t	msg_bytes;
 	atomic_t	msg_hdrs;
 	int		auto_msgmni;
diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 130dfec..b0e99de 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -62,7 +62,7 @@ static int proc_ipc_dointvec_minmax_orphans(ctl_table *table, int write,
 	return err;
 }
 
-static int proc_ipc_callback_dointvec(ctl_table *table, int write,
+static int proc_ipc_callback_dointvec_minmax(ctl_table *table, int write,
 	void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	struct ctl_table ipc_table;
@@ -72,7 +72,7 @@ static int proc_ipc_callback_dointvec(ctl_table *table, int write,
 	memcpy(&ipc_table, table, sizeof(ipc_table));
 	ipc_table.data = get_ipc(table);
 
-	rc = proc_dointvec(&ipc_table, write, buffer, lenp, ppos);
+	rc = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
 
 	if (write && !rc && lenp_bef == *lenp)
 		/*
@@ -152,15 +152,13 @@ static int proc_ipcauto_dointvec_minmax(ctl_table *table, int write,
 #define proc_ipc_dointvec	   NULL
 #define proc_ipc_dointvec_minmax   NULL
 #define proc_ipc_dointvec_minmax_orphans   NULL
-#define proc_ipc_callback_dointvec NULL
+#define proc_ipc_callback_dointvec_minmax  NULL
 #define proc_ipcauto_dointvec_minmax NULL
 #endif
 
 static int zero;
 static int one = 1;
-#ifdef CONFIG_CHECKPOINT_RESTORE
 static int int_max = INT_MAX;
-#endif
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -198,21 +196,27 @@ static struct ctl_table ipc_kern_table[] = {
 		.data		= &init_ipc_ns.msg_ctlmax,
 		.maxlen		= sizeof (init_ipc_ns.msg_ctlmax),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &int_max,
 	},
 	{
 		.procname	= "msgmni",
 		.data		= &init_ipc_ns.msg_ctlmni,
 		.maxlen		= sizeof (init_ipc_ns.msg_ctlmni),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_callback_dointvec,
+		.proc_handler	= proc_ipc_callback_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &int_max,
 	},
 	{
 		.procname	=  "msgmnb",
 		.data		= &init_ipc_ns.msg_ctlmnb,
 		.maxlen		= sizeof (init_ipc_ns.msg_ctlmnb),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &int_max,
 	},
 	{
 		.procname	= "sem",
-- 
1.7.10.4