linux-kernel - Re: [RFC v13][PATCH 05/14] x86 support for checkpoint/restart

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 24 Feb 2009 01:47:39 -0600
From:	Nathan Lynch <ntl@...ox.com>
To:	Oren Laadan <orenl@...columbia.edu>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-api@...r.kernel.org, containers@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org,
	Dave Hansen <dave@...ux.vnet.ibm.com>, linux-mm@...ck.org,
	Linus Torvalds <torvalds@...l.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [RFC v13][PATCH 05/14] x86 support for checkpoint/restart

Hi, this is an old thread I guess, but I just noticed some issues while
looking at this code.

On Tue, 27 Jan 2009 12:08:03 -0500
Oren Laadan <orenl@...columbia.edu> wrote:
> +static int cr_read_cpu_fpu(struct cr_ctx *ctx, struct task_struct *t)
> +{
> +	void *xstate_buf = cr_hbuf_get(ctx, xstate_size);
> +	int ret;
> +
> +	ret = cr_kread(ctx, xstate_buf, xstate_size);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* i387 + MMU + SSE */
> +	preempt_disable();
> +
> +	/* init_fpu() also calls set_used_math() */
> +	ret = init_fpu(current);
> +	if (ret < 0)
> +		return ret;

Several problems here:
* init_fpu can call kmem_cache_alloc(GFP_KERNEL), but is called here
  with preempt disabled (init_fpu could use a might_sleep annotation?)
* if init_fpu returns an error, we get preempt imbalance
* if init_fpu returns an error, we "leak" the cr_hbuf_get for
  xstate_buf

Speaking of cr_hbuf_get... I'd prefer to see that "allocator" go away
and its users converted to kmalloc/kfree (this is what I've done for
the powerpc C/R code, btw).

Using the slab allocator would:

* make the code less obscure and easier to review
* make the code more amenable to static analysis
* gain the benefits of slab debugging at runtime

But I think this has been pointed out before.  If I understand the
justification for cr_hbuf_get correctly, the allocations it services
are somehow known to be bounded in size and nesting.  But even if that
is the case, it's not much of a reason to avoid using kmalloc, is it?
--
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/