Re: [PATCH] extensions: add libxt_bpf extension

[Willem de Bruijn <willemb@google.com>]

On Mon, Jan 21, 2013 at 7:19 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jan 09, 2013 at 07:12:07PM -0500, Willem de Bruijn wrote:
>> Changes:
>> - v2
>>   - update to match latest kernel module (fixed size match struct)
>>   - add manual page
>>   - fix save/restore
>>   - fix matching whole program on delete
>>
>> Support filtering using Linux Socket Filters
>> ---
>>  extensions/libxt_bpf.c   |  199 ++++++++++++++++++++++++++++++++++++++++++++++
>>  extensions/libxt_bpf.man |   17 ++++
>>  2 files changed, 216 insertions(+), 0 deletions(-)
>>  create mode 100644 extensions/libxt_bpf.c
>>  create mode 100644 extensions/libxt_bpf.man
>>
>> diff --git a/extensions/libxt_bpf.c b/extensions/libxt_bpf.c
>> new file mode 100644
>> index 0000000..8ad540f
>> --- /dev/null
>> +++ b/extensions/libxt_bpf.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Xtables BPF extension
>> + *
>> + * Written by Willem de Bruijn (willemb@google.com)
>> + * Copyright Google, Inc. 2013
>> + * Licensed under the GNU General Public License version 2 (GPLv2)
>> +*/
>> +
>> +#include <linux/netfilter/xt_bpf.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +#include <xtables.h>
>> +
>> +#define BCODE_FILE_MAX_LEN_B 1024
>
> This has to be updated now that this sticks to 64 instructions per
> filter.

Oh.. rereading my comments after sending the revised patch I see that
I missed this. Will fix up in the next revision. If my little
arithmetic below is correct, it should be either 1603, or a little bit
larger (this is mainly a safety against enormous files).

>> +
>> +enum { O_BCODE_STDIN = 0,
>> +       O_BCODE_FILE};
>
> Better:
>
> enum {
>         O_...
>         O_...
> };

Done

>> +
>> +static void bpf_help(void)
>> +{
>> +     printf(
>> +"bpf match options:\n"
>> +"--bytecode-file <file>      : a bpf program as generated by\n"
>> +"  `tcpdump -i any -ddd <expr> > file\n"
>> +"--bytecode <program>        : a bpf program as generated by\n"
>> +"  `tcpdump -i any -ddd <expr> | tr '\\n' ','`\n");
>> +}
>> +
>> +static const struct xt_option_entry bpf_opts[] = {
>> +     {.name = "bytecode", .id = O_BCODE_STDIN, .type = XTTYPE_STRING},
>> +     {.name = "bytecode-file", .id = O_BCODE_FILE, .type = XTTYPE_STRING},
>> +     XTOPT_TABLEEND,
>> +};
>> +
>> +static void bpf_parse_string(struct xt_option_call *cb, const char *bpf_program,
>> +                          const char separator)
>> +{
>> +     struct xt_bpf_info *bi = (void *) cb->data;
>> +     const char *token;
>> +     char sp;
>> +     int i;
>> +
>> +     /* parse head: length. */
>> +     if (sscanf(bpf_program, "%hu%c", &bi->bpf_program_num_elem, &sp) != 2 ||
>> +                sp != separator)
>> +             xtables_error(PARAMETER_PROBLEM,
>> +                           "bpf: error parsing program length");
>> +     if (!bi->bpf_program_num_elem)
>> +             xtables_error(PARAMETER_PROBLEM,
>> +                           "bpf: illegal zero length program");
>> +     if (bi->bpf_program_num_elem > XT_BPF_MAX_NUM_INSTR)
>> +             xtables_error(PARAMETER_PROBLEM,
>> +                           "bpf: number of instructions exceeds maximum");
>> +
>> +     /* parse instructions. */
>> +     i = 0;
>> +     token = bpf_program;
>> +     while ((token = strchr(token, separator)) && (++token)[0]) {
>> +             if (i >= bi->bpf_program_num_elem) {
>> +                     xtables_error(PARAMETER_PROBLEM,
>> +                                   "bpf: program length: parameter");
>
> better error reporting?

Done

>> +                     return;

Done

> That return is superfluous. xtables_error calls exit.
>
>> +             }
>> +             if (sscanf(token, "%hu %hhu %hhu %u,",
>> +                        &bi->bpf_program[i].code,
>> +                        &bi->bpf_program[i].jt,
>> +                        &bi->bpf_program[i].jf,
>> +                        &bi->bpf_program[i].k) != 4) {
>> +                     xtables_error(PARAMETER_PROBLEM,
>> +                                   "bpf: error at instr %d", i);
>> +                     return;
>
> Same thing.

Done

>> +             }
>> +             i++;
>> +     }
>> +
>> +     if (i != bi->bpf_program_num_elem) {
>> +             xtables_error(PARAMETER_PROBLEM,
>> +                           "bpf: program length: parsing");
>
> Better error reporting?

Done

>> +             return;
>
> Same thing.

Done

>> +     }
>> +}
>> +
>> +static void bpf_parse_file(struct xt_option_call *cb, const char *filepath)
>> +{
>> +     struct stat stats;
>> +     char *contents;
>> +     int fd, len = 0, ret;
>> +
>> +     if (access(filepath, F_OK))
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: no such file");
>> +     if (access(filepath, R_OK))
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: no read permissions");
>
> Remove these above so...

Done

>
>> +
>> +     fd = open(filepath, O_RDONLY);
>> +     if (fd == -1)
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: error opening file");
>
> you can use strerror here.

Done

>
>> +     if (fstat(fd, &stats))
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: error reading metadata");
>> +     if (stats.st_size >= BCODE_FILE_MAX_LEN_B)
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: file too large");
>> +
>> +     contents = malloc(stats.st_size + 1);
>
> Better a buffer allocated in the stack now that we have 64
> instructions per filter?

The decimal notation still allows variable size, causing us to have to
allocate an upper estimate. This is 3 characters for the first line +
64 * (5 + 1 + 3 + 1 + 3 + 1 + 10 + 1) == 1603 B if I didn't mess up
somewhere.

> Or use fread to get each line and put it into the filter? so reading
> the filter and parsing it happens all at once.

That would involve duplicating some of the parsing logic from
bpf_parse_string, and the same error reports (program length mismatch,
instruction errors, etcetera). I think that parsing in only one
location is preferable (but I'm happy to change the code if you
prefer).

>> +     if (!contents)
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: allocation failed");
>> +     contents[stats.st_size] = 0;
>> +
>> +     while (len < stats.st_size) {
>> +             ret = read(fd, contents + len, stats.st_size - len);
>> +             if (ret == -1) {
>> +                     if (errno == EAGAIN)
>> +                             continue;
>> +                     xtables_error(PARAMETER_PROBLEM, "bpf: read failed");
>> +             }
>> +             if (!ret)
>> +                     xtables_error(PARAMETER_PROBLEM, "bpf: unexpected EOF");
>> +             len += ret;
>> +     }
>> +
>> +     bpf_parse_string(cb, contents, '\n');
>> +
>> +     free(contents);
>> +     close(fd);
>> +}
>> +
>> +static void bpf_parse(struct xt_option_call *cb)
>> +{
>> +     xtables_option_parse(cb);
>> +     switch (cb->entry->id) {
>> +     case O_BCODE_STDIN:
>> +             bpf_parse_string(cb, cb->arg, ',');
>> +             break;
>> +     case O_BCODE_FILE:
>> +             bpf_parse_file(cb, cb->arg);
>> +             break;
>> +     default:
>> +             xtables_error(PARAMETER_PROBLEM, "bpf: unknown option");
>> +     }
>> +}
>> +
>> +static void bpf_print_code(const void *ip, const struct xt_entry_match *match)
>> +{
>> +     const struct xt_bpf_info *info = (void *) match->data;
>> +     int i;
>> +
>> +     for (i = 0; i < info->bpf_program_num_elem; i++)
>> +             printf("%hu %hhu %hhu %u,", info->bpf_program[i].code,
>> +                                         info->bpf_program[i].jt,
>> +                                         info->bpf_program[i].jf,
>> +                                         info->bpf_program[i].k);
>> +}
>> +
>> +static void bpf_save(const void *ip, const struct xt_entry_match *match)
>> +{
>> +     const struct xt_bpf_info *info = (void *) match->data;
>> +
>> +     printf(" --bytecode \"%hu,", info->bpf_program_num_elem);
>> +     bpf_print_code(ip, match);
>> +     printf("\"");
>> +}
>> +
>> +static void bpf_fcheck(struct xt_fcheck_call *cb)
>> +{
>> +     if (((!!(cb->xflags & (1 << O_BCODE_STDIN))) ^
>> +          (!!(cb->xflags & (1 << O_BCODE_FILE)))) == 0)
>> +             xtables_error(PARAMETER_PROBLEM,
>> +                           "bpf: pass bytecode or bytecode-file, not both");
>> +}
>> +
>> +static void bpf_print(const void *ip, const struct xt_entry_match *match,
>> +                   int numeric)
>> +{
>> +     printf("match bpf ");
>> +     return bpf_print_code(ip, match);
>> +}
>> +
>> +static struct xtables_match bpf_match = {
>> +     .family         = NFPROTO_UNSPEC,
>> +     .name           = "bpf",
>> +     .version        = XTABLES_VERSION,
>> +     .size           = XT_ALIGN(sizeof(struct xt_bpf_info)),
>> +     .userspacesize  = XT_ALIGN(sizeof(struct xt_bpf_info) - sizeof(void *)),
>
> Use offsetof instead, please.

Done.

>> +     .help           = bpf_help,
>> +     .print          = bpf_print,
>> +     .save           = bpf_save,
>> +     .x6_parse       = bpf_parse,
>> +     .x6_fcheck      = bpf_fcheck,
>> +     .x6_options     = bpf_opts,
>> +};
>> +
>> +void _init(void)
>> +{
>> +     xtables_register_match(&bpf_match);
>> +}
>> diff --git a/extensions/libxt_bpf.man b/extensions/libxt_bpf.man
>> new file mode 100644
>> index 0000000..359684c
>> --- /dev/null
>> +++ b/extensions/libxt_bpf.man
>> @@ -0,0 +1,17 @@
>> +.TP
>> +Match using Linux Socket Filter. Expects a BPF program in decimal format for a device without link layer. This is the format generated by \fBtcpdump \-ddd \-i $DEV\fP, if $DEV is of type DLT_RAW.
>> +.TP
>> +\fB\-\-bytecode\-file\fP \fIfilename\fP
>> +Pass the program stored in a file.
>> +For example:
>> +.IP
>> +iptables \-A OUTPUT \-m bpf \-\-bytecode-file program.bpf \-j ACCEPT
>
> It's a good idea to describe the format of the file here.

Done. Maybe a bit too verbose now. Let me know, if so.

>> +.TP
>> +\fB\-\-bytecode\fP \fIcode\fP
>> +Pass the program on the command line. In this case all end-lines
>> +must have been converted to commas (for instance using `tr '\\n' ','`).
>> +For example:
>> +.IP
>> +iptables \-A OUTPUT \-m bpf \-\-bytecode
>> +         '6,40 0 0 14, 21 0 3 2048,48 0 0 25,21 0 1 20,6 0 0 96,6 0 0 0'
>
> Please, a filter that actually works ;-)

Oops! Done (and tested).

>> +         \-j ACCEPT
>> --
>> 1.7.7.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks!