Messages in this thread | | | From | KP Singh <> | Date | Tue, 17 Nov 2020 02:55:03 +0100 | Subject | Re: [PATCH bpf-next v2 2/2] bpf: Add tests for bpf_lsm_set_bprm_opts |
| |
On Tue, Nov 17, 2020 at 1:43 AM Martin KaFai Lau <kafai@fb.com> wrote: > > On Mon, Nov 16, 2020 at 11:25:36PM +0000, KP Singh wrote: > > From: KP Singh <kpsingh@google.com> > > > > The test forks a child process, updates the local storage to set/unset > > the securexec bit. > > > > The BPF program in the test attaches to bprm_creds_for_exec which checks > > the local storage of the current task to set the secureexec bit on the > > binary parameters (bprm). > > > > The child then execs a bash command with the environment variable > > TMPDIR set in the envp. The bash command returns a different exit code > > based on its observed value of the TMPDIR variable. > > > > Since TMPDIR is one of the variables that is ignored by the dynamic > > loader when the secureexec bit is set, one should expect the > > child execution to not see this value when the secureexec bit is set. > > > > Signed-off-by: KP Singh <kpsingh@google.com> > > --- > > .../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++ > > tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++ > > 2 files changed, 158 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > new file mode 100644 > > index 000000000000..cba1ef3dc8b4 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c > > @@ -0,0 +1,124 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Copyright (C) 2020 Google LLC. > > + */ > > + > > +#include <asm-generic/errno-base.h> > > +#include <sys/stat.h> > Is it needed?
No, Good catch, removed.
> > > +#include <test_progs.h>
[...]
> > + * If the value of TMPDIR is set, the bash command returns 10 > > + * and if the value is unset, it returns 20. > > + */ > > + ret = execle("/bin/bash", "bash", "-c", > > + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20", > > + NULL, bash_envp); > > + if (ret)
> It should never reach here? May be just exit() unconditionally > instead of having a chance to fall-through and then return -EINVAL.
Agreed. changed it to exit(errno); here.
> > > + exit(errno); > > + } else if (child_pid > 0) { > > + waitpid(child_pid, &child_status, 0); > > + ret = WEXITSTATUS(child_status); > > + > > + /* If a secureexec occured, the exit status should be 20. > > + */ > > + if (secureexec && ret == 20) > > + return 0; > > + > > + /* If normal execution happened the exit code should be 10. > > + */ > > + if (!secureexec && ret == 10) > > + return 0; > > + > > + return ret; > Any chance that ret may be 0?
I think it's safer to just let it fall through and return -EINVAL, so I removed the return ret here.
> > > + }
[...]
> > + 0 /* secureexec */); > > + if (CHECK(err, "run_set_secureexec:0", "err = %d", err)) > nit. err = %d"\n"
Fixed.
> > > + goto close_prog; > > + > > + /* Run the test with the secureexec bit set */ > > + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map), > > + 1 /* secureexec */); > > + if (CHECK(err, "run_set_secureexec:1", "err = %d", err)) > Same here.
Fixed.
- KP
> > Others LGTM.
| |