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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
