From: Arjun Roy <hidden> Date: 2021-02-06 20:37:43
From: Arjun Roy <redacted>
Explicitly define reserved field and require it to be 0-valued.
Fixes: 7eeba1706eba ("tcp: Add receive timestamp support for receive zerocopy.")
Signed-off-by: Arjun Roy <redacted>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Soheil Hassas Yeganeh <redacted>
Suggested-by: David Ahern <redacted>
Suggested-by: Leon Romanovsky <leon@kernel.org>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
---
include/uapi/linux/tcp.h | 2 +-
net/ipv4/tcp.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
@@ -357,6 +357,6 @@ struct tcp_zerocopy_receive {__u64msg_control;/* ancillary data */__u64msg_controllen;__u32msg_flags;-/* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */+__u32reserved;/* set to 0 for now */};#endif /* _UAPI_LINUX_TCP_H */
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
I was expecting we'd also throw in a check_zeroed_user().
Either we can check if the buffer is zeroed all the way,
or we can't and we shouldn't validate reserved either
check_zeroed_user(optval + offsetof(reserved),
len - offsetof(reserved))
?
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
I was expecting we'd also throw in a check_zeroed_user().
Either we can check if the buffer is zeroed all the way,
or we can't and we shouldn't validate reserved either
check_zeroed_user(optval + offsetof(reserved),
len - offsetof(reserved))
?
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Thanks
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
I was expecting we'd also throw in a check_zeroed_user().
Either we can check if the buffer is zeroed all the way,
or we can't and we shouldn't validate reserved either
check_zeroed_user(optval + offsetof(reserved),
len - offsetof(reserved))
?
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
I was expecting we'd also throw in a check_zeroed_user().
Either we can check if the buffer is zeroed all the way,
or we can't and we shouldn't validate reserved either
check_zeroed_user(optval + offsetof(reserved),
len - offsetof(reserved))
?
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-09 02:54:26
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
On 2/8/21 11:41 AM, Jakub Kicinski wrote:
quoted
On Sun, 7 Feb 2021 10:26:54 +0200 Leon Romanovsky wrote:
quoted
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
From: David Ahern <hidden> Date: 2021-02-09 03:23:35
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
On 2/8/21 11:41 AM, Jakub Kicinski wrote:
quoted
On Sun, 7 Feb 2021 10:26:54 +0200 Leon Romanovsky wrote:
quoted
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
So, in short I think the "if (zc.reserved)" is correct as Leon noted.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-02-09 06:30:26
On Mon, Feb 08, 2021 at 08:20:29PM -0700, David Ahern wrote:
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
quoted
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
On 2/8/21 11:41 AM, Jakub Kicinski wrote:
quoted
On Sun, 7 Feb 2021 10:26:54 +0200 Leon Romanovsky wrote:
quoted
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-09 17:00:11
On Mon, 8 Feb 2021 20:20:29 -0700 David Ahern wrote:
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
quoted
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
Let me try one last time.
There is no check in the kernels that len <= N. User can pass any
length _already_. check_zeroed_user() forces the values beyond the
structure length to be known (0) rather than anything. It can only
avoid breakages in the future.
So, in short I think the "if (zc.reserved)" is correct as Leon noted.
If it's correct to check some arbitrary part of the buffer is zeroed
it should be correct to check the entire tail is zeroed.
From: Arjun Roy <hidden> Date: 2021-02-10 00:08:48
On Tue, Feb 9, 2021 at 8:59 AM Jakub Kicinski [off-list ref] wrote:
On Mon, 8 Feb 2021 20:20:29 -0700 David Ahern wrote:
quoted
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
quoted
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
Let me try one last time.
There is no check in the kernels that len <= N. User can pass any
length _already_. check_zeroed_user() forces the values beyond the
structure length to be known (0) rather than anything. It can only
avoid breakages in the future.
quoted
So, in short I think the "if (zc.reserved)" is correct as Leon noted.
If it's correct to check some arbitrary part of the buffer is zeroed
it should be correct to check the entire tail is zeroed.
So, coming back to the thread, I think the following appears to be the
current thoughts:
1. It is requested that, on the kernel as it stands today, fields
beyond zc.msg_flags (including zc.reserved, the only such field as of
this patch) are zero'd out. So a new userspace asking to do specific
things would fail on this old kernel with EINVAL. Old userspace would
work on old or new kernels. New of course works on new kernels.
2. If it's correct to check some arbitrary field (zc.reserved) to be
0, then it should be fine to check this for all future fields >=
reserved in the struct. So some advanced userspace down the line
doesn't get confused.
Strictly speaking, I'm not convinced this is necessary - eg. 64 bytes
struct right now, suppose userspace of the future gives us 96 bytes of
which the last 32 are non-zero for some feature or the other. We, in
the here and now kernel, truncate that length to 64 (as in we only
copy to kernel those first 64 bytes) and set the returned length to
64. The understanding being, any (future, past or present) userspace
consults the output value; and considers anything byte >= the returned
len to be untouched by the kernel executing the call (ie. garbage,
unacted upon).
So, how would this work for old+new userspace on old+new kernel?
A) old+old, new+new: sizes match, no issue
B) new kernel, old userspace: That's not an issue. We have the
switch(len) statement for that.
C) old kernel, new userspace: that's the 96 vs. 64 B example above -
new userspace would see that the kernel only operated on 64 B and
treat the last 32 B as garbage/unacted on.
In this case, we would not give EINVAL on case C, as we would if we
returned EINVAL on a check_zeroed_user() case for fields past
zc.reserved. We'd do a zerocopy operating on just the features we know
about, and communicate to the user that we only acted on features up
until this byte offset.
Now, given this is the case, we still have the padding confusion with
zc.reserved and the current struct size, so we have to force it to 0
as we are doing. But I think we don't need to go beyond this so far.
Thus, my personal preference is to not have the check_zeroed_user()
check. But if the consensus demands it, then it's an easy enough fix.
What are your thoughts?
Thanks,
-Arjun
From: David Ahern <hidden> Date: 2021-02-10 04:36:32
On 2/9/21 4:46 PM, Arjun Roy wrote:
On Tue, Feb 9, 2021 at 8:59 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Mon, 8 Feb 2021 20:20:29 -0700 David Ahern wrote:
quoted
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
quoted
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
Let me try one last time.
There is no check in the kernels that len <= N. User can pass any
length _already_. check_zeroed_user() forces the values beyond the
structure length to be known (0) rather than anything. It can only
avoid breakages in the future.
quoted
So, in short I think the "if (zc.reserved)" is correct as Leon noted.
If it's correct to check some arbitrary part of the buffer is zeroed
it should be correct to check the entire tail is zeroed.
So, coming back to the thread, I think the following appears to be the
current thoughts:
1. It is requested that, on the kernel as it stands today, fields
beyond zc.msg_flags (including zc.reserved, the only such field as of
this patch) are zero'd out. So a new userspace asking to do specific
things would fail on this old kernel with EINVAL. Old userspace would
work on old or new kernels. New of course works on new kernels.
2. If it's correct to check some arbitrary field (zc.reserved) to be
0, then it should be fine to check this for all future fields >=
reserved in the struct. So some advanced userspace down the line
doesn't get confused.
Strictly speaking, I'm not convinced this is necessary - eg. 64 bytes
struct right now, suppose userspace of the future gives us 96 bytes of
which the last 32 are non-zero for some feature or the other. We, in
the here and now kernel, truncate that length to 64 (as in we only
copy to kernel those first 64 bytes) and set the returned length to
64. The understanding being, any (future, past or present) userspace
consults the output value; and considers anything byte >= the returned
len to be untouched by the kernel executing the call (ie. garbage,
unacted upon).
So, how would this work for old+new userspace on old+new kernel?
A) old+old, new+new: sizes match, no issue
B) new kernel, old userspace: That's not an issue. We have the
switch(len) statement for that.
C) old kernel, new userspace: that's the 96 vs. 64 B example above -
new userspace would see that the kernel only operated on 64 B and
treat the last 32 B as garbage/unacted on.
In this case, we would not give EINVAL on case C, as we would if we
returned EINVAL on a check_zeroed_user() case for fields past
zc.reserved. We'd do a zerocopy operating on just the features we know
about, and communicate to the user that we only acted on features up
until this byte offset.
Now, given this is the case, we still have the padding confusion with
zc.reserved and the current struct size, so we have to force it to 0
as we are doing. But I think we don't need to go beyond this so far.
Thus, my personal preference is to not have the check_zeroed_user()
check. But if the consensus demands it, then it's an easy enough fix.
What are your thoughts?
bpf uses check_zeroed_user to make sure extensions to its structs are
compatible, so yes, this is required.
Also, you need to address legitimate msg_flags as I mentioned in another
response.
From: Arjun Roy <hidden> Date: 2021-02-10 19:25:25
On Tue, Feb 9, 2021 at 8:35 PM David Ahern [off-list ref] wrote:
On 2/9/21 4:46 PM, Arjun Roy wrote:
quoted
On Tue, Feb 9, 2021 at 8:59 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Mon, 8 Feb 2021 20:20:29 -0700 David Ahern wrote:
quoted
On 2/8/21 7:53 PM, Jakub Kicinski wrote:
quoted
On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
quoted
That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.
Indeed, so we're agreeing that check_zeroed_user() is needed before
original optlen from user space gets truncated?
I thought so, but maybe not. To think through this ...
If current kernel understands a struct of size N, it can only copy that
amount from user to kernel. Anything beyond is ignored in these
multiplexed uAPIs, and that is where the new userspace on old kernel falls.
Known value checks can only be done up to size N. In this case, the
reserved field is at the end of the known struct size, so checking just
the field is fine. Going beyond the reserved field has implications for
extensions to the API which should be handled when those extensions are
added.
Let me try one last time.
There is no check in the kernels that len <= N. User can pass any
length _already_. check_zeroed_user() forces the values beyond the
structure length to be known (0) rather than anything. It can only
avoid breakages in the future.
quoted
So, in short I think the "if (zc.reserved)" is correct as Leon noted.
If it's correct to check some arbitrary part of the buffer is zeroed
it should be correct to check the entire tail is zeroed.
So, coming back to the thread, I think the following appears to be the
current thoughts:
1. It is requested that, on the kernel as it stands today, fields
beyond zc.msg_flags (including zc.reserved, the only such field as of
this patch) are zero'd out. So a new userspace asking to do specific
things would fail on this old kernel with EINVAL. Old userspace would
work on old or new kernels. New of course works on new kernels.
2. If it's correct to check some arbitrary field (zc.reserved) to be
0, then it should be fine to check this for all future fields >=
reserved in the struct. So some advanced userspace down the line
doesn't get confused.
Strictly speaking, I'm not convinced this is necessary - eg. 64 bytes
struct right now, suppose userspace of the future gives us 96 bytes of
which the last 32 are non-zero for some feature or the other. We, in
the here and now kernel, truncate that length to 64 (as in we only
copy to kernel those first 64 bytes) and set the returned length to
64. The understanding being, any (future, past or present) userspace
consults the output value; and considers anything byte >= the returned
len to be untouched by the kernel executing the call (ie. garbage,
unacted upon).
So, how would this work for old+new userspace on old+new kernel?
A) old+old, new+new: sizes match, no issue
B) new kernel, old userspace: That's not an issue. We have the
switch(len) statement for that.
C) old kernel, new userspace: that's the 96 vs. 64 B example above -
new userspace would see that the kernel only operated on 64 B and
treat the last 32 B as garbage/unacted on.
In this case, we would not give EINVAL on case C, as we would if we
returned EINVAL on a check_zeroed_user() case for fields past
zc.reserved. We'd do a zerocopy operating on just the features we know
about, and communicate to the user that we only acted on features up
until this byte offset.
Now, given this is the case, we still have the padding confusion with
zc.reserved and the current struct size, so we have to force it to 0
as we are doing. But I think we don't need to go beyond this so far.
Thus, my personal preference is to not have the check_zeroed_user()
check. But if the consensus demands it, then it's an easy enough fix.
What are your thoughts?
bpf uses check_zeroed_user to make sure extensions to its structs are
compatible, so yes, this is required.
Very well; I shall send out a v3 patch with this.
Also, you need to address legitimate msg_flags as I mentioned in another
response.
I meant to respond to this earlier but forgot. v3 will address this as well.
-Arjun
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
I was expecting we'd also throw in a check_zeroed_user().
Either we can check if the buffer is zeroed all the way,
or we can't and we shouldn't validate reserved either
check_zeroed_user(optval + offsetof(reserved),
len - offsetof(reserved))
?
There is a check that len is not larger than zs and users can't give
large buffer.
I would say that is pretty safe to write "if (zc.reserved)".
Which check? There's a check which truncates (writes back to user space
len = min(len, sizeof(zc)). Application can still pass garbage beyond
sizeof(zc) and syscall may start failing in the future if sizeof(zc)
changes.
At least in my tree, we have the length check:
4155 if (len > sizeof(zc)) {
4156 len = sizeof(zc);
4157 if (put_user(len, optlen))
4158 return -EFAULT;
4159 }
Ad David wrote below, the "if (zc.reserved)" is enough.
We have following options:
1. Old kernel that have sizeof(sz) upto .reserved and old userspace
-> len <= sizeof(sz) - works correctly.
2. Old kernel that have sizeof(sz) upto .reserved and new userspace that
sends larger struct -> "f (len > sizeof(zc))" will return -EFAULT
3. New kernel that have sizeof(sz) beyond reserved and old userspace
-> any new added field to struct sz should be checked and anyway it is the same as item 1.
4. New kernel and new userspace
-> standard flow.
Thanks
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-09 17:00:22
On Tue, 9 Feb 2021 08:15:11 +0200 Leon Romanovsky wrote:
At least in my tree, we have the length check:
4155 if (len > sizeof(zc)) {
4156 len = sizeof(zc);
4157 if (put_user(len, optlen))
4158 return -EFAULT;
4159 }
Ad David wrote below, the "if (zc.reserved)" is enough.
We have following options:
1. Old kernel that have sizeof(sz) upto .reserved and old userspace
-> len <= sizeof(sz) - works correctly.
2. Old kernel that have sizeof(sz) upto .reserved and new userspace that
sends larger struct -> "f (len > sizeof(zc))" will return -EFAULT
Based on the code you quoted? I don't see how. Maybe I need a vacation.
put_user() just copies len back to user space after truncation.
3. New kernel that have sizeof(sz) beyond reserved and old userspace
-> any new added field to struct sz should be checked and anyway it is the same as item 1.
4. New kernel and new userspace
-> standard flow.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-02-09 21:12:47
On Tue, Feb 09, 2021 at 08:59:38AM -0800, Jakub Kicinski wrote:
On Tue, 9 Feb 2021 08:15:11 +0200 Leon Romanovsky wrote:
quoted
At least in my tree, we have the length check:
4155 if (len > sizeof(zc)) {
4156 len = sizeof(zc);
4157 if (put_user(len, optlen))
4158 return -EFAULT;
4159 }
Ad David wrote below, the "if (zc.reserved)" is enough.
We have following options:
1. Old kernel that have sizeof(sz) upto .reserved and old userspace
-> len <= sizeof(sz) - works correctly.
2. Old kernel that have sizeof(sz) upto .reserved and new userspace that
sends larger struct -> "f (len > sizeof(zc))" will return -EFAULT
Based on the code you quoted? I don't see how. Maybe I need a vacation.
put_user() just copies len back to user space after truncation.
It is me who needs to go to vacation, you are right it doesn't return
for lengths larger than sizeof(zc).
quoted
3. New kernel that have sizeof(sz) beyond reserved and old userspace
-> any new added field to struct sz should be checked and anyway it is the same as item 1.
4. New kernel and new userspace
-> standard flow.
From: David Ahern <hidden> Date: 2021-02-07 17:50:48
On 2/6/21 1:36 PM, Arjun Roy wrote:
quoted hunk
From: Arjun Roy <redacted>
Explicitly define reserved field and require it to be 0-valued.
Fixes: 7eeba1706eba ("tcp: Add receive timestamp support for receive zerocopy.")
Signed-off-by: Arjun Roy <redacted>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Soheil Hassas Yeganeh <redacted>
Suggested-by: David Ahern <redacted>
Suggested-by: Leon Romanovsky <leon@kernel.org>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
---
include/uapi/linux/tcp.h | 2 +-
net/ipv4/tcp.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
@@ -357,6 +357,6 @@ struct tcp_zerocopy_receive {__u64msg_control;/* ancillary data */__u64msg_controllen;__u32msg_flags;-/* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */+__u32reserved;/* set to 0 for now */};#endif /* _UAPI_LINUX_TCP_H */
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
The 'switch (len)' statement needs to be updated now that 'len' is not
going to end on the 'msg_flags' boundary? But then, how did that work
before if there was 4 byte padding?
Maybe I am missing something here. You currently have:
switch (len) {
case offsetofend(struct tcp_zerocopy_receive, msg_flags):
which should == 60, yet the struct size is 64 with 4-bytes of padding. A
user doing
int optlen = sizeof(struct tcp_zerocopy_receive);
getsockopt(...., &optlen)
would pass in a value of 64, right?
From: David Ahern <hidden> Date: 2021-02-07 17:54:27
On 2/7/21 10:49 AM, David Ahern wrote:
On 2/6/21 1:36 PM, Arjun Roy wrote:
quoted
From: Arjun Roy <redacted>
Explicitly define reserved field and require it to be 0-valued.
Fixes: 7eeba1706eba ("tcp: Add receive timestamp support for receive zerocopy.")
Signed-off-by: Arjun Roy <redacted>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Soheil Hassas Yeganeh <redacted>
Suggested-by: David Ahern <redacted>
Suggested-by: Leon Romanovsky <leon@kernel.org>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
---
include/uapi/linux/tcp.h | 2 +-
net/ipv4/tcp.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
@@ -357,6 +357,6 @@ struct tcp_zerocopy_receive {__u64msg_control;/* ancillary data */__u64msg_controllen;__u32msg_flags;-/* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */+__u32reserved;/* set to 0 for now */};#endif /* _UAPI_LINUX_TCP_H */
@@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,}if(copy_from_user(&zc,optval,len))return-EFAULT;+if(zc.reserved)+return-EINVAL;lock_sock(sk);err=tcp_zerocopy_receive(sk,&zc,&tss);release_sock(sk);
The 'switch (len)' statement needs to be updated now that 'len' is not
going to end on the 'msg_flags' boundary? But then, how did that work
before if there was 4 byte padding?
Maybe I am missing something here. You currently have:
switch (len) {
case offsetofend(struct tcp_zerocopy_receive, msg_flags):
Ah, I missed the lines before it:
if (len >= offsetofend(struct tcp_zerocopy_receive,
msg_flags))
goto zerocopy_rcv_cmsg;
Also, I see a check on zc.msg_flags for a specific flag option being
set. What about other invalid bits in msg_flags? I do not see a check like:
#define TCP_VALID_ZC_MSG_FLAGS (TCP_CMSG_TS)
if (zc.msg_flags & ~(TCP_VALID_ZC_MSG_FLAGS))
return -EINVAL;