Re: speaking of stacks

[David Miller <davem@davemloft.net>]

From: jamal <hadi@cyberus.ca>
Date: Mon, 31 Mar 2008 09:15:56 -0400

> 
> All this talk about stacks got me thirsty.
> Heres something thats a mystery to me; probably very basic
> when doing static check analysis (kernel make checkstack)
> 
> I have ipsec turned on in the linus git tree kernel and
> net/xfrm/xfrm_user.c::xfrm_get_policy() shows up as one of
> the top offenders. Eyeballing reveals that
> struct xfrm_policy seems to be the offender 
> in the else {} after if (p->index) in that function.
> That seems understandable - once variable "tmp" is declared it goes
> on the stack and it is freakin huge. 
> However, checkstack only seems to catch it if i have the
> line "memset(&tmp, 0, sizeof(struct xfrm_policy))". If i remove
> that line - even though tmp is on the stack - checkstack doesnt catch
> it.
> 
> What gives?

You probably have most of the security infrastructure turned
off, therefore GCC can see that 'tmp' is basically unused
and can therefore be totally eliminated.

The memset() call makes 'tmp' get passed by reference to
another function, and thus become used.

This whole song and dance here is for SELINUX to set only
the policy->security, so that we can pass that back down
into the subsequent xfrm_policy_bysel_ctx().

The thing to do is to rearrange these security layer hooks
so that they take a "struct xfrm_sec_ctx **" instead of
a full policy pointer.

Then the code would look like:

		struct nlattr *rt = attrs[XFRMA_SEC_CTX];
		struct xfrm_sec_ctx *ctx;

		err = verify_sec_ctx_len(attrs);
		if (err)
			return err;

		if (rt) {
			struct xfrm_user_sec_ctx *uctx = nla_data(rt);

			if ((err = security_xfrm_policy_alloc(&ctx, uctx)))
				return err;
		}
		xp = xfrm_policy_bysel_ctx(type, p->dir, &p->sel, ctx,
					   delete, &err);
		security_xfrm_policy_free(ctx);

And thus the xfrm_policy wouldn't need to be on the stack
any longer.