Re: sys_sendmsg Fails Silently With Negative msg_namelen

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Mar 07, 2014 at 07:39:55PM +0000, Matthew Leach wrote:
> Hello,
> 
> Passing -1 in msg->msg_namelen to sys_sendmsg will cause the syscall
> to finish without error. This happens because of the following check
> in copy_msghdr_from_user:
> 
> if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> 	 kmsg->msg_namelen = sizeof(struct sockaddr_storage);
> 
> This check passes due to a comparison between signed (msg_namelen =
> -1) and unsigned values (sizeof(struct sockaddr_storage) = 128). This
> was introduced with 1661bf36 ("net: heap overflow in
> __audit_sockaddr()").

The silent capping was actually introduced in commit db31c55a6fb2 ('net:
clamp ->msg_namelen instead of returning an error').  Just returning an
error code broke beta versions of Ruby and maybe something else?

> 
> Below is an ugly patch that fixes this. Are there any suggestions on a
> cleaner fix?

Your patch re-introduces the memory corruption bug that 1661bf36 ("net:
heap overflow in __audit_sockaddr()") was supposed to fix.

I think Ruby was using larger buffer sizes than necessary so we could
add something like:

	if (kmsg->msg_namelen < 0)
		return -EINVAL;
	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
  		kmsg->msg_namelen = sizeof(struct sockaddr_storage);

Why are people passing -1 as the buffer size anyway?  Your email
suggests that people expect it to work, and it will work fine if you
have a buffer size which is larger than sizeof(struct sockaddr_storage).
I'm nervous about changing something which works fine in case I break
userspace.  A second time.  :P

regards,
dan carpenter