Message ID | 3aa068fa482f7cf5381957e9a3ea58550822d1d1.1483555162.git.sowmini.varadhan@oracle.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On 01/04/2017 07:45 PM, Sowmini Varadhan wrote: > The bpf_prog used in sock_setfilter() only attempts to check for > ip pktlen, and verifies that the contents of the 80'th packet in > the ethernet frame is 'a' or 'b'. Thus many non-udp packets > could incorrectly pass through this filter and cause incorrect > test results. > > This commit hardens the conditions checked by the filter so > that only UDP/IPv4 packets with the matching length and test-character > will be permitted by the filter. The filter has been cleaned up > to explicitly use the BPF macros to make it more readable. > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> > Acked-by: Willem de Bruijn <willemb@google.com> > --- > v2: commit comment edited based on Willem de Bruijn review > v3: Shuah Khan nit. > > tools/testing/selftests/net/psock_lib.h | 29 ++++++++++++++++++++++------- > 1 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h > index 24bc7ec..9e5553a 100644 > --- a/tools/testing/selftests/net/psock_lib.h > +++ b/tools/testing/selftests/net/psock_lib.h > @@ -27,6 +27,7 @@ > #include <string.h> > #include <arpa/inet.h> > #include <unistd.h> > +#include <netinet/udp.h> > > #define DATA_LEN 100 > #define DATA_CHAR 'a' > @@ -40,14 +41,28 @@ > > static __maybe_unused void sock_setfilter(int fd, int lvl, int optnum) > { > + uint16_t ip_len = DATA_LEN + > + sizeof(struct iphdr) + > + sizeof(struct udphdr); > + /* the filter below checks for all of the following conditions that > + * are based on the contents of create_payload() > + * ether type 0x800 and > + * ip proto udp and > + * ip len == ip_len and > + * udp[38] == 'a' or udp[38] == 'b' > + */ > struct sock_filter bpf_filter[] = { > - { 0x80, 0, 0, 0x00000000 }, /* LD pktlen */ > - { 0x35, 0, 4, DATA_LEN }, /* JGE DATA_LEN [f goto nomatch]*/ > - { 0x30, 0, 0, 0x00000050 }, /* LD ip[80] */ > - { 0x15, 1, 0, DATA_CHAR }, /* JEQ DATA_CHAR [t goto match]*/ > - { 0x15, 0, 1, DATA_CHAR_1}, /* JEQ DATA_CHAR_1 [t goto match]*/ > - { 0x06, 0, 0, 0x00000060 }, /* RET match */ > - { 0x06, 0, 0, 0x00000000 }, /* RET no match */ > + BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 12), /* LD ethertype */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ETH_P_IP, 0, 8), > + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 23), /* LD ip_proto */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, IPPROTO_UDP, 0, 6), > + BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 16), /* LD ip_len */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ip_len, 0, 4), > + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 80), /* LD udp[38] */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR, 1, 0), > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR_1, 0, 1), > + BPF_STMT(BPF_RET | BPF_K, ~0), /* match */ > + BPF_STMT(BPF_RET | BPF_K, 0) /* no match */ Just reading up on the thread, sorry to jump in late. Can't you just use the generated code from bpf_asm (tools/net/) and add the asm program as a comment above? Something like we do in net/core/ptp_classifier.c +13. As it stands it makes it a bit harder to parse / less readable with macros actually. Rest seems fine, thanks. > }; > struct sock_fprog bpf_prog; > >
On (01/04/17 23:16), Daniel Borkmann wrote: > > Just reading up on the thread, sorry to jump in late. Can't you just > use the generated code from bpf_asm (tools/net/) and add the asm program > as a comment above? Something like we do in net/core/ptp_classifier.c +13. I was actually using the example from the BSD bpf(4) man page, and expanding on that one.. https://www.freebsd.org/cgi/man.cgi?query=bpf&sektion=4&manpath=FreeBSD+4.7-RELEASE (I could not find the equivalent linux man page). It was a lot easier to parse than the existing code . > As it stands it makes it a bit harder to parse / less readable with macros > actually. Rest seems fine, thanks. You think the earlier code was readable? I had to use gcc -E, with help from the bpf(4) page, to make sense of it. --Sowmini
On 01/04/2017 11:22 PM, Sowmini Varadhan wrote: > On (01/04/17 23:16), Daniel Borkmann wrote: >> >> Just reading up on the thread, sorry to jump in late. Can't you just >> use the generated code from bpf_asm (tools/net/) and add the asm program >> as a comment above? Something like we do in net/core/ptp_classifier.c +13. > > I was actually using the example from the BSD bpf(4) man page, > and expanding on that one.. > https://www.freebsd.org/cgi/man.cgi?query=bpf&sektion=4&manpath=FreeBSD+4.7-RELEASE > (I could not find the equivalent linux man page). > > It was a lot easier to parse than the existing code . cBPF with its tooling is all documented here: Documentation/networking/filter.txt >> As it stands it makes it a bit harder to parse / less readable with macros >> actually. Rest seems fine, thanks. > > You think the earlier code was readable? I had to use > gcc -E, with help from the bpf(4) page, to make sense of it. > > --Sowmini > >
On 01/04/2017 11:45 AM, Sowmini Varadhan wrote: > The bpf_prog used in sock_setfilter() only attempts to check for > ip pktlen, and verifies that the contents of the 80'th packet in > the ethernet frame is 'a' or 'b'. Thus many non-udp packets > could incorrectly pass through this filter and cause incorrect > test results. > > This commit hardens the conditions checked by the filter so > that only UDP/IPv4 packets with the matching length and test-character > will be permitted by the filter. The filter has been cleaned up > to explicitly use the BPF macros to make it more readable. > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> > Acked-by: Willem de Bruijn <willemb@google.com> > --- > v2: commit comment edited based on Willem de Bruijn review > v3: Shuah Khan nit. > > tools/testing/selftests/net/psock_lib.h | 29 ++++++++++++++++++++++------- > 1 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h > index 24bc7ec..9e5553a 100644 > --- a/tools/testing/selftests/net/psock_lib.h > +++ b/tools/testing/selftests/net/psock_lib.h > @@ -27,6 +27,7 @@ > #include <string.h> > #include <arpa/inet.h> > #include <unistd.h> > +#include <netinet/udp.h> > > #define DATA_LEN 100 > #define DATA_CHAR 'a' > @@ -40,14 +41,28 @@ > > static __maybe_unused void sock_setfilter(int fd, int lvl, int optnum) > { > + uint16_t ip_len = DATA_LEN + > + sizeof(struct iphdr) + > + sizeof(struct udphdr); > + /* the filter below checks for all of the following conditions that > + * are based on the contents of create_payload() > + * ether type 0x800 and > + * ip proto udp and > + * ip len == ip_len and > + * udp[38] == 'a' or udp[38] == 'b' > + */ Looks like you have to do v4 anyway, please make sure your comment block is one of the acceptable formats based on coding style: https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2 > struct sock_filter bpf_filter[] = { > - { 0x80, 0, 0, 0x00000000 }, /* LD pktlen */ > - { 0x35, 0, 4, DATA_LEN }, /* JGE DATA_LEN [f goto nomatch]*/ > - { 0x30, 0, 0, 0x00000050 }, /* LD ip[80] */ > - { 0x15, 1, 0, DATA_CHAR }, /* JEQ DATA_CHAR [t goto match]*/ > - { 0x15, 0, 1, DATA_CHAR_1}, /* JEQ DATA_CHAR_1 [t goto match]*/ > - { 0x06, 0, 0, 0x00000060 }, /* RET match */ > - { 0x06, 0, 0, 0x00000000 }, /* RET no match */ > + BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 12), /* LD ethertype */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ETH_P_IP, 0, 8), > + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 23), /* LD ip_proto */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, IPPROTO_UDP, 0, 6), > + BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 16), /* LD ip_len */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ip_len, 0, 4), > + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 80), /* LD udp[38] */ > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR, 1, 0), > + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR_1, 0, 1), > + BPF_STMT(BPF_RET | BPF_K, ~0), /* match */ > + BPF_STMT(BPF_RET | BPF_K, 0) /* no match */ > }; > struct sock_fprog bpf_prog; > >
On (01/04/17 23:26), Daniel Borkmann wrote: > > >>As it stands it makes it a bit harder to parse / less readable with macros > >>actually. Rest seems fine, thanks. Usually macros are there (a) as an abstraction so you dont have to hard-code things, and, (b) to make things more readable. (maybe that's why the 1992 VJ paper on BPF came up with these macros?) I think we differ on code-aesthetics (not correctness) here. It was not immediately obvious to me that "0x15 is actually BPF_JMP + BPF_JEQ + BPF_K" etc, when I wanted to extend the bpf_prog to harden the checks in the existing code. Would it be ok to leave the extremely subjective "make this more readable" part for you to tackle later? Or I can just drop patch1, and you can fix it to your satisfaction later. --Sowmini
On (01/04/17 15:37), Shuah Khan wrote: > Looks like you have to do v4 anyway, please make sure your comment > block is one of the acceptable formats based on coding style: I'm not sure about that. I can just keep patch 2. thanks, --Sowmini
On (01/04/17 15:37), Shuah Khan wrote: > > + /* the filter below checks for all of the following conditions that > > + * are based on the contents of create_payload() > > + * ether type 0x800 and > > + * ip proto udp and > > + * ip len == ip_len and > > + * udp[38] == 'a' or udp[38] == 'b' > > + */ > > Looks like you have to do v4 anyway, please make sure your comment > block is one of the acceptable formats based on coding style: > > https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2 BTW, the above is conformant with the comment style required for networking: https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt which seems to be used in psock_fanout.c and reuseport_bpf.c as well. Thanks --Sowmini
On 01/04/2017 11:48 PM, Sowmini Varadhan wrote: > On (01/04/17 23:26), Daniel Borkmann wrote: [...] >>>> As it stands it makes it a bit harder to parse / less readable with macros >>>> actually. Rest seems fine, thanks. > > Usually macros are there (a) as an abstraction so you > dont have to hard-code things, and, (b) to make things > more readable. (maybe that's why the 1992 VJ paper on > BPF came up with these macros?) > > I think we differ on code-aesthetics (not correctness) here. > It was not immediately obvious to me that "0x15 is actually > BPF_JMP + BPF_JEQ + BPF_K" etc, when I wanted to extend > the bpf_prog to harden the checks in the existing code. > > Would it be ok to leave the extremely subjective > "make this more readable" part for you to tackle later? > Or I can just drop patch1, and you can fix it to your > satisfaction later. I think we're talking past each other (?), my suggestion from my original email was to use bpf_asm and paste the (human readable) program as a comment above as done also elsewhere. But just leave it as it is then, no big deal either.
On 01/04/2017 03:55 PM, Sowmini Varadhan wrote: > On (01/04/17 15:37), Shuah Khan wrote: >>> + /* the filter below checks for all of the following conditions that >>> + * are based on the contents of create_payload() >>> + * ether type 0x800 and >>> + * ip proto udp and >>> + * ip len == ip_len and >>> + * udp[38] == 'a' or udp[38] == 'b' >>> + */ >> >> Looks like you have to do v4 anyway, please make sure your comment >> block is one of the acceptable formats based on coding style: >> >> https://marc.info/?l=linux-crypto-vger&m=146799837129319&w=2 > > BTW, the above is conformant with the comment style required for > networking: > > https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt > > which seems to be used in psock_fanout.c and reuseport_bpf.c as well. I would like to see the comment blocks in selftest consistent with the Kernel coding style. > > Thanks > --Sowmini > Could you please split this patch into two. Hardening part in one and the cleanup in a separate patch. This way I can get the hardening fix into 4.10 in my next Kselftest update. Cleanup patch can go in later. thanks, -- Shuah
On (01/04/17 16:26), Shuah Khan wrote: > > Could you please split this patch into two. Hardening part in one and > the cleanup in a separate patch. This way I can get the hardening fix > into 4.10 in my next Kselftest update. Cleanup patch can go in later. > > thanks, > -- Shuah I'm a little confused by the comments above. Dan's suggestion was that I could have used some other tool to generate the code, rather than hand-crafting it as I did. In his last message, he suggests that it may be ok to leave the hand-crafted version as is (for now), as well. To make it clear: the current v3 version *is* the "hardening" part. Dan's suggestion is that the hand-crafted version can be replaced by bpf_asm generated code later. That would be the "cleanup" part, which I was going to do in a later commit. Does that help? --Sowmini
On 01/05/2017 08:54 AM, Sowmini Varadhan wrote: > On (01/04/17 16:26), Shuah Khan wrote: >> >> Could you please split this patch into two. Hardening part in one and >> the cleanup in a separate patch. This way I can get the hardening fix >> into 4.10 in my next Kselftest update. Cleanup patch can go in later. >> >> thanks, >> -- Shuah > > I'm a little confused by the comments above. > > Dan's suggestion was that I could have used some other > tool to generate the code, rather than hand-crafting it as I did. > In his last message, he suggests that it may be ok to leave > the hand-crafted version as is (for now), as well. > > To make it clear: > the current v3 version *is* the "hardening" part. Dan's suggestion is > that the hand-crafted version can be replaced by bpf_asm generated code > later. That would be the "cleanup" part, which I was going to do in a > later commit. > > Does that help? > > --Sowmini > Let's try this again. I want to see a separate patch for the filter cleanup. I don't want that included in the non-udp packet check. Please address the readability review comments from me and Daniel when you send your next version. thanks, -- Shuah
diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h index 24bc7ec..9e5553a 100644 --- a/tools/testing/selftests/net/psock_lib.h +++ b/tools/testing/selftests/net/psock_lib.h @@ -27,6 +27,7 @@ #include <string.h> #include <arpa/inet.h> #include <unistd.h> +#include <netinet/udp.h> #define DATA_LEN 100 #define DATA_CHAR 'a' @@ -40,14 +41,28 @@ static __maybe_unused void sock_setfilter(int fd, int lvl, int optnum) { + uint16_t ip_len = DATA_LEN + + sizeof(struct iphdr) + + sizeof(struct udphdr); + /* the filter below checks for all of the following conditions that + * are based on the contents of create_payload() + * ether type 0x800 and + * ip proto udp and + * ip len == ip_len and + * udp[38] == 'a' or udp[38] == 'b' + */ struct sock_filter bpf_filter[] = { - { 0x80, 0, 0, 0x00000000 }, /* LD pktlen */ - { 0x35, 0, 4, DATA_LEN }, /* JGE DATA_LEN [f goto nomatch]*/ - { 0x30, 0, 0, 0x00000050 }, /* LD ip[80] */ - { 0x15, 1, 0, DATA_CHAR }, /* JEQ DATA_CHAR [t goto match]*/ - { 0x15, 0, 1, DATA_CHAR_1}, /* JEQ DATA_CHAR_1 [t goto match]*/ - { 0x06, 0, 0, 0x00000060 }, /* RET match */ - { 0x06, 0, 0, 0x00000000 }, /* RET no match */ + BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 12), /* LD ethertype */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ETH_P_IP, 0, 8), + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 23), /* LD ip_proto */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, IPPROTO_UDP, 0, 6), + BPF_STMT(BPF_LD|BPF_H|BPF_ABS, 16), /* LD ip_len */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, ip_len, 0, 4), + BPF_STMT(BPF_LD|BPF_B|BPF_ABS, 80), /* LD udp[38] */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR, 1, 0), + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, DATA_CHAR_1, 0, 1), + BPF_STMT(BPF_RET | BPF_K, ~0), /* match */ + BPF_STMT(BPF_RET | BPF_K, 0) /* no match */ }; struct sock_fprog bpf_prog;