----- 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():
--- 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
