Re: sys_sendmsg Fails Silently With Negative msg_namelen

Re: sys_sendmsg Fails Silently With Negative msg_namelen

[Dan Carpenter]

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

Re: sys_sendmsg Fails Silently With Negative msg_namelen

[Matthew Leach]

Hi Dan,

Dan Carpenter <dan.carpenter@oracle.com> writes:

[...]

> 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);

I don't see how your patch does anything different?  If we don't clamp
the value and leave it as -1 the check for a negative buffer size
eventually happens in move_addr_to_kernel anyway, just before we copy
the buffer from userspace.  This check fails and returns EINVAL.

>
>
> Why are people passing -1 as the buffer size anyway?  

This was actually found with LTP.  The sendmsg01 test passes -1 as the
msg_namelen parameter and expects the syscall to fail.

> 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

Agreed, but IMHO passing -1 as a buffer size should cause a syscall to
fail, rather than assuming we can copy from the buffer.

-- 
Matt