Re: [PATCH v5 2/3] fs/binfmts: Better handling of binfmt loops

[Oleg Nesterov <oleg@redhat.com>]

On 08/15, Zach Levis wrote:
>
> +static bool update_prev_binfmts(struct linux_binprm *bprm,
> +				struct linux_binfmt *cur_fmt)
> +{
> +
> +	if (!try_module_get(cur_fmt->module))
> +		return false;
> +	if (bprm->previous_binfmts[1])
> +		put_binfmt(bprm->previous_binfmts[1]);
> +	bprm->previous_binfmts[1] = bprm->previous_binfmts[0];
> +	bprm->previous_binfmts[0] = cur_fmt;
> +	return true;
> +}

Still can't understand the logic behind this function and its usage.
IOW, what ->previous_binfmts[] actually means? previous_binfmts[1]
could be a caller or the previous fmt which was called at the same
depth.

> @@ -1393,15 +1498,38 @@ int search_binary_handler(struct linux_binprm *bprm)
>  	list_for_each_entry(fmt, &formats, lh) {
>  		if (!try_module_get(fmt->module))
>  			continue;
> +
> +		if (!update_prev_binfmts(bprm, fmt))
> +			continue;
> +
>  		read_unlock(&binfmt_lock);
> +
>  		bprm->recursion_depth++;
>  		retval = fmt->load_binary(bprm);
>  		bprm->recursion_depth--;
> -		if (retval >= 0 || retval != -ENOEXEC ||
> -		    bprm->mm == NULL || bprm->file == NULL) {
> +		if (retval == -ELOOP
> +		    && bprm->recursion_depth == 0) { /* cur, previous */
> +			pr_err("Too much recursion with binfmts (0:%s, -1:%s) in file %s, skipping (base %s).\n",
> +				binfmt_name(bprm->previous_binfmts[0]),
> +				binfmt_name(bprm->previous_binfmts[1]),
> +				bprm->filename,
> +				fmt->name);
> +
> +			free_arg_pages(bprm);
> +			if (bprm->interp != bprm->filename)
> +				kfree(bprm->interp);

this doesn't look safe too, kfree(interp) can be called twice.

and once again, we should not lose -ELOOP as an error code if the
next fmt returns ENOEXEC.

But the main problem (in my opinion) is that this doesn't worth the
trouble, sorry.

Oleg.