Re: [PATCH bpf-next v2 1/2] bpftool: Use appropriate permissions for map access

[Slava Imameev <slava.imameev@crowdstrike.com>]

> > Modify several functions in tools/bpf/bpftool/common.c to allow
> > specification of requested access for file descriptors, such as
> > read-only access.
> >
> > Update bpftool to request only read access for maps when write
> > access is not required. This fixes errors when reading from maps
> > that are protected from modification via security_bpf_map.
> >
> > Signed-off-by: Slava Imameev <slava.imameev@crowdstrike.com>
>
>
> Thanks for this!
>
> I think the topic of map access in bpftool has been discussed in the
> path, but I can't remember what we said or find it again - maybe I don't
> remember correctly. Looks good to me overall.
>
> One question: How thoroughly have you tested that write permissions are
> necessary for the different cases? I'm asking because I'm wondering
> whether we could restrict to read-only in a couple more cases, see
> below. (At the end of the day it doesn't matter too much, it's fine
> being conservative and conserving write permissions for now, we can
> always refine later; it's already an improvement to do read-only for the
> dump/list cases).


The goal of this patch was to fix bpftool errors we experienced on our systems.
The efforts were focused only on changes to the affected subset of map commands.


> > +		/* Get an fd with the requested options. */
> > +		close(fd);
> > +		fd = bpf_map_get_fd_by_id_opts(id, opts);
> > +		if (fd < 0) {
> > +			p_err("can't get map by id (%u): %s", id,
> > +			      strerror(errno));
> > +			goto err_close_fds;
> > +		}
>
>
> We could maybe skip this step if the requested options are read-only, no
> need to close and re-open a fd in that case?


I agree. The change will be submitted with version 3.


> > -int map_parse_fds(int *argc, char ***argv, int **fds)
> > +int map_parse_fds(int *argc, char ***argv, int **fds, __u32 open_flags)
> >  {
> > +	LIBBPF_OPTS(bpf_get_fd_by_id_opts, opts);
> > +
> > +	if (open_flags & ~BPF_F_RDONLY) {
> > +		p_err("invalid open_flags: %x", open_flags);
> > +		return -1;
> > +	}
>
>
> I don't think we need this check, the flag is never passed by users. If
> you want to catch a bug, use an assert() instead?


I agree. This check is replaced with an assert and will be submitted with v3.


> > diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
> > index 5c39c2ed36a2..ad318a8667a4 100644
> > --- a/tools/bpf/bpftool/iter.c
> > +++ b/tools/bpf/bpftool/iter.c
> > @@ -37,7 +37,7 @@ static int do_pin(int argc, char **argv)
> >  				return -1;
> >  			}
> >  
> > -			map_fd = map_parse_fd(&argc, &argv);
> > +			map_fd = map_parse_fd(&argc, &argv, 0);
>
>
> Do you need write permissions here? (I don't remember.)


Iterator requires only read access. I changed it to BPF_F_RDONLY for v3.
An iterator test is added to v3.


> > -	fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
> > +	fd = map_parse_fd_and_info(&argc, &argv, &info, &len, BPF_F_RDONLY);
>
>
> This one is surprising, don't you need write permissions to delete an
> element from the map? Please double-check if you haven't already, I
> wouldn't want to break "bpftool map delete".
>
> I note you don't test items deletion in your tests, by the way.


Right, the delete command requires write access. I changed it and added
an item deletion test to v3.


> >  static int do_pin(int argc, char **argv)
> >  {
> >  	int err;
> >  
> > -	err = do_pin_any(argc, argv, map_parse_fd);
> > +	err = do_pin_any(argc, argv, map_parse_read_only_fd);
> >  	if (!err && json_output)
> >  		jsonw_null(json_wtr);
> >  	return err;
> > @@ -1319,7 +1329,7 @@ static int do_create(int argc, char **argv)
> >  			if (!REQ_ARGS(2))
> >  				usage();
> >  			inner_map_fd = map_parse_fd_and_info(&argc, &argv,
> > -							     &info, &len);
> > +							     &info, &len, 0);
>
>
> Do you need write permissions for the inner map's fd? This is something
> that could be worth checking in the tests, as well.

The inner map fd can be created with read only access. I changed it and added
a test for map-of-maps creation to v3.


> > @@ -128,7 +128,8 @@ int do_event_pipe(int argc, char **argv)
> >  	int err, map_fd;
> >  
> >  	map_info_len = sizeof(map_info);
> > -	map_fd = map_parse_fd_and_info(&argc, &argv, &map_info, &map_info_len);
> > +	map_fd = map_parse_fd_and_info(&argc, &argv, &map_info, &map_info_len,
> > +				       0);
>
>
> This one might be worth checking, too.


An event pipe map fd requires write access as the map is updated by bpf_map_update_elem
inside __perf_buffer__new .