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

[Mathias Krause]

On 3 November 2013 01:35, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, Nov 2, 2013 at 2:26 PM, Mathias Krause <minipli@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/