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

Message ID 1383427599-1980-2-git-send-email-minipli@googlemail.com
State New, archived
Headers show
Series
  • IPC DoS fix
Related show

Commit Message

Mathias Krause Nov. 2, 2013, 9:26 p.m. UTC
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(-)

Comments

Linus Torvalds Nov. 3, 2013, 12:35 a.m. UTC | #1
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
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 *);
Mathias Krause Nov. 3, 2013, 8:36 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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;