diff mbox series

[bpf-next,v8,04/12] bpf: Only reply field should be writeable

Message ID 20180124075802.1522053-5-brakmo@fb.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series bpf: More sock_ops callbacks | expand

Commit Message

Lawrence Brakmo Jan. 24, 2018, 7:57 a.m. UTC
Currently, a sock_ops BPF program can write the op field and all the
reply fields (reply and replylong). This is a bug. The op field should
not have been writeable and there is currently no way to use replylong
field for indices >= 1. This patch enforces that only the reply field
(which equals replylong[0]) is writeable.

Fixes: 40304b2a1567 ("bpf: BPF support for sock_ops")
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
 net/core/filter.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Yuchung Cheng Jan. 24, 2018, 7:58 p.m. UTC | #1
On Tue, Jan 23, 2018 at 11:57 PM, Lawrence Brakmo <brakmo@fb.com> wrote:
> Currently, a sock_ops BPF program can write the op field and all the
> reply fields (reply and replylong). This is a bug. The op field should
> not have been writeable and there is currently no way to use replylong
> field for indices >= 1. This patch enforces that only the reply field
> (which equals replylong[0]) is writeable.
Would this patch be more suitable for -net ?

>
> Fixes: 40304b2a1567 ("bpf: BPF support for sock_ops")
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
Acked-by: Yuchung Cheng <ycheng@google.com>

> ---
>  net/core/filter.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 0cf170f..c356ec0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3845,8 +3845,7 @@ static bool sock_ops_is_valid_access(int off, int size,
>  {
>         if (type == BPF_WRITE) {
>                 switch (off) {
> -               case offsetof(struct bpf_sock_ops, op) ...
> -                    offsetof(struct bpf_sock_ops, replylong[3]):
> +               case offsetof(struct bpf_sock_ops, reply):
>                         break;
>                 default:
>                         return false;
> --
> 2.9.5
>
Alexei Starovoitov Jan. 24, 2018, 8:23 p.m. UTC | #2
On 1/24/18 11:58 AM, Yuchung Cheng wrote:
> On Tue, Jan 23, 2018 at 11:57 PM, Lawrence Brakmo <brakmo@fb.com> wrote:
>> Currently, a sock_ops BPF program can write the op field and all the
>> reply fields (reply and replylong). This is a bug. The op field should
>> not have been writeable and there is currently no way to use replylong
>> field for indices >= 1. This patch enforces that only the reply field
>> (which equals replylong[0]) is writeable.
> Would this patch be more suitable for -net ?

yes. we will send it to 4.15 and 4.14 as soon as it's released.
See discussion with Eric.

>>
>> Fixes: 40304b2a1567 ("bpf: BPF support for sock_ops")
>> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
>
>> ---
>>  net/core/filter.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 0cf170f..c356ec0 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3845,8 +3845,7 @@ static bool sock_ops_is_valid_access(int off, int size,
>>  {
>>         if (type == BPF_WRITE) {
>>                 switch (off) {
>> -               case offsetof(struct bpf_sock_ops, op) ...
>> -                    offsetof(struct bpf_sock_ops, replylong[3]):
>> +               case offsetof(struct bpf_sock_ops, reply):
>>                         break;
>>                 default:
>>                         return false;
>> --
>> 2.9.5
>>
diff mbox series

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index 0cf170f..c356ec0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3845,8 +3845,7 @@  static bool sock_ops_is_valid_access(int off, int size,
 {
 	if (type == BPF_WRITE) {
 		switch (off) {
-		case offsetof(struct bpf_sock_ops, op) ...
-		     offsetof(struct bpf_sock_ops, replylong[3]):
+		case offsetof(struct bpf_sock_ops, reply):
 			break;
 		default:
 			return false;