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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 16 Aug 2013 15:15:01 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Zach Levis <zach@...hsthings.com>
Cc:	akpm@...ux-foundation.org, viro@...iv.linux.org.uk,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	dan.carpenter@...cle.com, keescook@...omium.org,
	cody@...ux.vnet.ibm.com, zml@...ux.vnet.ibm.com
Subject: Re: [PATCH v5 2/3] fs/binfmts: Better handling of binfmt loops

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/