Messages in this thread | | | Date | Wed, 10 Oct 2018 18:14:47 +1100 | From | Aleksa Sarai <> | Subject | Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace |
| |
On 2018-10-09, Laurent Vivier <laurent@vivier.eu> wrote: > Le 09/10/2018 à 17:16, Tycho Andersen a écrit : > > On Tue, Oct 09, 2018 at 12:37:52PM +0200, Laurent Vivier wrote: > >> @@ -80,18 +74,32 @@ static int entry_count; > >> */ > >> #define MAX_REGISTER_LENGTH 1920 > >> > >> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns) > >> +{ > >> + struct binfmt_namespace *b_ns; > >> + > >> + while (ns) { > >> + b_ns = READ_ONCE(ns->binfmt_ns); > >> + if (b_ns) > >> + return b_ns; > >> + ns = ns->parent; > >> + } > >> + WARN_ON_ONCE(1); > > > > It looks like we warn here, > > > >> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm) > >> struct file *interp_file = NULL; > >> int retval; > >> int fd_binary = -1; > >> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns()); > >> > >> retval = -ENOEXEC; > >> - if (!enabled) > >> + if (!ns->enabled) > > > > ...but then in cases like this we immediately dereference the pointer > > anyways and crash. Can we return some other error code here in the !ns > > case so we don't crash? > > My concern here is I don't want to add code to check an error case that > cannot happen. The first namespace binfmt_ns pointer is initialized with > &init_binfmt_ns, so the return value cannot be NULL.
I'd argue that BUG() is a better thing to do then -- if doing a dummy error path makes no sense. Though IIRC BUG() is no longer a popular thing to do.
-- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/> [unhandled content-type:application/pgp-signature] | |