* Re: sys_sendmsg Fails Silently With Negative msg_namelen [not found] <87vbvpx0fo.fsf@e106496-lin.cambridge.arm.com> @ 2014-03-07 21:26 ` Dan Carpenter 2014-03-10 10:48 ` Matthew Leach 0 siblings, 1 reply; 2+ messages in thread From: Dan Carpenter @ 2014-03-07 21:26 UTC (permalink / raw) To: Matthew Leach; +Cc: netdev, David S. Miller, Will Deacon 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: sys_sendmsg Fails Silently With Negative msg_namelen 2014-03-07 21:26 ` sys_sendmsg Fails Silently With Negative msg_namelen Dan Carpenter @ 2014-03-10 10:48 ` Matthew Leach 0 siblings, 0 replies; 2+ messages in thread From: Matthew Leach @ 2014-03-10 10:48 UTC (permalink / raw) To: Dan Carpenter; +Cc: netdev@vger•kernel.org, David S. Miller, Will Deacon 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-03-10 10:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <87vbvpx0fo.fsf@e106496-lin.cambridge.arm.com>
2014-03-07 21:26 ` sys_sendmsg Fails Silently With Negative msg_namelen Dan Carpenter
2014-03-10 10:48 ` Matthew Leach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox