Message ID | 20190320173332.18105-2-alban@kinvolk.io |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next,v1,1/7] tools: bpftool: fix infinite loop | expand |
On Wed, 20 Mar 2019 18:33:27 +0100, Alban Crequy wrote: > From: Alban Crequy <alban@kinvolk.io> > > Symptoms: > $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries > Error: '8' needs at least 2 arguments, 1 found > > After this patch: > $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries > Error: can't parse max entries: missing argument > > Signed-off-by: Alban Crequy <alban@kinvolk.io> > --- > tools/bpf/bpftool/common.c | 5 +++++ > tools/bpf/bpftool/main.h | 11 +++++++---- > tools/bpf/bpftool/map.c | 15 ++++++++++++--- > tools/bpf/bpftool/prog.c | 2 -- > 4 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c > index f7261fad45c1..e560cd8f66bc 100644 > --- a/tools/bpf/bpftool/common.c > +++ b/tools/bpf/bpftool/common.c > @@ -627,6 +627,11 @@ int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what) > return -1; > } > > + if (*argc == 0) { > + p_err("can't parse %s: missing argument", what); > + return -1; > + } > > *val = strtoul(**argv, &endptr, 0); > if (*endptr) { > p_err("can't parse %s as %s", **argv, what); > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > index d7dd84d3c660..7fc2973446d0 100644 > --- a/tools/bpf/bpftool/main.h > +++ b/tools/bpf/bpftool/main.h > @@ -22,14 +22,17 @@ > #define NEXT_ARGP() ({ (*argc)--; (*argv)++; if (*argc < 0) usage(); }) > #define BAD_ARG() ({ p_err("what is '%s'?", *argv); -1; }) > #define GET_ARG() ({ argc--; *argv++; }) > -#define REQ_ARGS(cnt) \ > + > +#define REQ_ARGS(cnt) REQ_ARGS_GENERIC(cnt, argc, argv) > +#define REQ_ARGSP(cnt) REQ_ARGS_GENERIC(cnt, *argc, *argv) > +#define REQ_ARGS_GENERIC(cnt, ARGC, ARGV) \ > ({ \ > int _cnt = (cnt); \ > bool _res; \ > \ > - if (argc < _cnt) { \ > - p_err("'%s' needs at least %d arguments, %d found", \ > - argv[-1], _cnt, argc); \ > + if ((ARGC) < _cnt) { \ > + p_err("'%s' needs at least %d arguments, %d found", \ > + (ARGV)[-1], _cnt, (ARGC)); \ > _res = false; \ > } else { \ > _res = true; \ > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > index 994a7e0d16fb..18f9bc3aed4f 100644 > --- a/tools/bpf/bpftool/map.c > +++ b/tools/bpf/bpftool/map.c > @@ -98,6 +98,9 @@ int map_parse_fd(int *argc, char ***argv) > char *endptr; > > NEXT_ARGP(); > + if (!REQ_ARGSP(1)) { > + return -1; > + } > > id = strtoul(**argv, &endptr, 0); > if (*endptr) { > @@ -114,6 +117,9 @@ int map_parse_fd(int *argc, char ***argv) > char *path; > > NEXT_ARGP(); > + if (!REQ_ARGSP(1)) { > + return -1; > + } > > path = **argv; > NEXT_ARGP(); > @@ -1100,11 +1106,10 @@ static int do_create(int argc, char **argv) > pinfile = GET_ARG(); > > while (argc) { > - if (!REQ_ARGS(2)) > - return -1; Seems like it'd be better to make a version of REQ_ARGS() which uses the next arg in the error message (if it exists) rather than pushing all the checks out? I think the format of the parsing loop is quite canonical here, with all arguments being in the <keyword> <value> form, therefore I think it's worthwhile improving the REQ_ARGS(2) check. > if (is_prefix(*argv, "type")) { > NEXT_ARG(); > + if (!REQ_ARGS(1)) > + return -1; > > if (attr.map_type) { > p_err("map type already specified"); > @@ -1119,6 +1124,8 @@ static int do_create(int argc, char **argv) > NEXT_ARG(); > } else if (is_prefix(*argv, "name")) { > NEXT_ARG(); > + if (!REQ_ARGS(1)) > + return -1; > attr.name = GET_ARG(); > } else if (is_prefix(*argv, "key")) { > if (parse_u32_arg(&argc, &argv, &attr.key_size, > @@ -1138,6 +1145,8 @@ static int do_create(int argc, char **argv) > return -1; > } else if (is_prefix(*argv, "dev")) { > NEXT_ARG(); > + if (!REQ_ARGS(1)) > + return -1; > > if (attr.map_ifindex) { > p_err("offload device already specified"); > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 8ef80d65a474..e0875ef5e444 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -836,8 +836,6 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd, > } > > NEXT_ARG(); > - if (!REQ_ARGS(2)) > - return -EINVAL; > > *mapfd = map_parse_fd(&argc, &argv); > if (*mapfd < 0)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index f7261fad45c1..e560cd8f66bc 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -627,6 +627,11 @@ int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what) return -1; } + if (*argc == 0) { + p_err("can't parse %s: missing argument", what); + return -1; + } + *val = strtoul(**argv, &endptr, 0); if (*endptr) { p_err("can't parse %s as %s", **argv, what); diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index d7dd84d3c660..7fc2973446d0 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -22,14 +22,17 @@ #define NEXT_ARGP() ({ (*argc)--; (*argv)++; if (*argc < 0) usage(); }) #define BAD_ARG() ({ p_err("what is '%s'?", *argv); -1; }) #define GET_ARG() ({ argc--; *argv++; }) -#define REQ_ARGS(cnt) \ + +#define REQ_ARGS(cnt) REQ_ARGS_GENERIC(cnt, argc, argv) +#define REQ_ARGSP(cnt) REQ_ARGS_GENERIC(cnt, *argc, *argv) +#define REQ_ARGS_GENERIC(cnt, ARGC, ARGV) \ ({ \ int _cnt = (cnt); \ bool _res; \ \ - if (argc < _cnt) { \ - p_err("'%s' needs at least %d arguments, %d found", \ - argv[-1], _cnt, argc); \ + if ((ARGC) < _cnt) { \ + p_err("'%s' needs at least %d arguments, %d found", \ + (ARGV)[-1], _cnt, (ARGC)); \ _res = false; \ } else { \ _res = true; \ diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index 994a7e0d16fb..18f9bc3aed4f 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -98,6 +98,9 @@ int map_parse_fd(int *argc, char ***argv) char *endptr; NEXT_ARGP(); + if (!REQ_ARGSP(1)) { + return -1; + } id = strtoul(**argv, &endptr, 0); if (*endptr) { @@ -114,6 +117,9 @@ int map_parse_fd(int *argc, char ***argv) char *path; NEXT_ARGP(); + if (!REQ_ARGSP(1)) { + return -1; + } path = **argv; NEXT_ARGP(); @@ -1100,11 +1106,10 @@ static int do_create(int argc, char **argv) pinfile = GET_ARG(); while (argc) { - if (!REQ_ARGS(2)) - return -1; - if (is_prefix(*argv, "type")) { NEXT_ARG(); + if (!REQ_ARGS(1)) + return -1; if (attr.map_type) { p_err("map type already specified"); @@ -1119,6 +1124,8 @@ static int do_create(int argc, char **argv) NEXT_ARG(); } else if (is_prefix(*argv, "name")) { NEXT_ARG(); + if (!REQ_ARGS(1)) + return -1; attr.name = GET_ARG(); } else if (is_prefix(*argv, "key")) { if (parse_u32_arg(&argc, &argv, &attr.key_size, @@ -1138,6 +1145,8 @@ static int do_create(int argc, char **argv) return -1; } else if (is_prefix(*argv, "dev")) { NEXT_ARG(); + if (!REQ_ARGS(1)) + return -1; if (attr.map_ifindex) { p_err("offload device already specified"); diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 8ef80d65a474..e0875ef5e444 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -836,8 +836,6 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd, } NEXT_ARG(); - if (!REQ_ARGS(2)) - return -EINVAL; *mapfd = map_parse_fd(&argc, &argv); if (*mapfd < 0)