On 12/04/2013 06:55 PM, Jan Stancek wrote: > > ----- Original Message ----- >> From: "Wanlong Gao" <[email protected]> >> To: "Cyril Hrubis" <[email protected]>, "Jan Stancek" <[email protected]> >> Cc: "LTP List" <[email protected]> >> Sent: Wednesday, 4 December, 2013 4:01:02 AM >> Subject: sendmsg01 (invalid to buffer length) FAIL >> >> Hi Jan, Cyril, >> >> Before the following commit[1], we assigned "tolen=-1" to test >> it as a invalid to buffer length, but treated as unsigned int, >> (-1 > sizeof(struct sockaddr_storage)) is true, so it >> returns -1 with errno=EINVAL to us. AFTER this commit[1], to avoid overflow, >> the invalid buffer length is reassigned to a valid one. Then the return value >> is 0, unexpected success for our test case. >> >> Any thoughts? > > Hi, > > so "msg_namelen" defined in kernel (include/linux/socket.h) is a signed int, > but userspace defines it as unsigned int (socklen_t), which brings the > question > if negative msg_namelen has any sense. > > 1.) We can propose a patch to preserve old behaviour where negative > msg_namelen > stays negative: > > - if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) > + if (kmsg->msg_namelen > 0 && > + kmsg->msg_namelen > sizeof(struct sockaddr_storage)) > kmsg->msg_namelen = sizeof(struct sockaddr_storage); > > 2.) If 1) is not accepted because negative values don't make much sense, > we can change .tolen to "1", which is still invalid and will hit EINVAL > later at udp_sendmsg():
I think 1) should not be accepted, we'd better go to 2). So, could you please push a patch with change log on below "tolen" change? Thanks, Wanlong Gao Reported-by: Fengguang Wu <[email protected]> Tested-by: Wanlong Gao <[email protected]> > > --- a/testcases/kernel/syscalls/sendmsg/sendmsg01.c > +++ b/testcases/kernel/syscalls/sendmsg/sendmsg01.c > @@ -211,7 +211,7 @@ struct test_case_t tdat[] = { > .msg = &msgdat, > .flags = 0, > .to = (struct sockaddr *)&sin1, > - .tolen = -1, > + .tolen = 1, > .retval = -1, > .experrno = EINVAL, > .setup = setup1, > > Regards, > Jan > >> >> >> Thanks, >> Wanlong Gao >> >> >> >> >> [1] >> From: Dan Carpenter <[email protected]> >> Date: Wed, 27 Nov 2013 15:40:21 +0300 >> Subject: [PATCH] net: clamp ->msg_namelen instead of returning an error >> >> If kmsg->msg_namelen > sizeof(struct sockaddr_storage) then in the >> original code that would lead to memory corruption in the kernel if you >> had audit configured. If you didn't have audit configured it was >> harmless. >> >> There are some programs such as beta versions of Ruby which use too >> large of a buffer and returning an error code breaks them. We should >> clamp the ->msg_namelen value instead. >> >> Fixes: 1661bf364ae9 ("net: heap overflow in __audit_sockaddr()") >> Reported-by: Eric Wong <[email protected]> >> Signed-off-by: Dan Carpenter <[email protected]> >> Tested-by: Eric Wong <[email protected]> >> Acked-by: Eric Dumazet <[email protected]> >> Signed-off-by: David S. Miller <[email protected]> >> --- >> net/compat.c | 2 +- >> net/socket.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/compat.c b/net/compat.c >> index 618c6a8..dd32e34 100644 >> --- a/net/compat.c >> +++ b/net/compat.c >> @@ -72,7 +72,7 @@ int get_compat_msghdr(struct msghdr *kmsg, struct >> compat_msghdr __user *umsg) >> __get_user(kmsg->msg_flags, &umsg->msg_flags)) >> return -EFAULT; >> if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) >> - return -EINVAL; >> + kmsg->msg_namelen = sizeof(struct sockaddr_storage); >> kmsg->msg_name = compat_ptr(tmp1); >> kmsg->msg_iov = compat_ptr(tmp2); >> kmsg->msg_control = compat_ptr(tmp3); >> diff --git a/net/socket.c b/net/socket.c >> index 0b18693..e83c416 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -1973,7 +1973,7 @@ static int copy_msghdr_from_user(struct msghdr *kmsg, >> if (copy_from_user(kmsg, umsg, sizeof(struct msghdr))) >> return -EFAULT; >> if (kmsg->msg_namelen > sizeof(struct sockaddr_storage)) >> - return -EINVAL; >> + kmsg->msg_namelen = sizeof(struct sockaddr_storage); >> return 0; >> } >> > ------------------------------------------------------------------------------ Sponsored by Intel(R) XDK Develop, test and display web and hybrid apps with a single code base. Download it for free now! http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk _______________________________________________ Ltp-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
