speaking of stacks

speaking of stacks

[jamal]

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?

----
hadi@lilsol:~$ gcc -v
Using built-in specs.
Target: i486-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c
++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared
--with-system-zlib --libexecdir=/usr/lib --without-included-gettext
--enable-threads=posix --enable-nls --program-suffix=-4.1
--enable-__cxa_atexit --enable-clocale=gnu --enable-libstdcxx-debug
--enable-mpfr --with-tune=i686 --enable-checking=release i486-linux-gnu
Thread model: posix
gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
-----

Not a gcc guru by far.
Anyone interested, I can send you the .o before and after commenting
for your perusal.

cheers,
jamal

Re: speaking of stacks

[David Miller]

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.

Re: speaking of stacks

[jamal]

On Thu, 2008-03-04 at 14:18 -0700, David Miller wrote:

> 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.

Indeed, thanks - that resolves the mystery;->

Testing by moving the tmp memseting inside security_xfrm_policy_alloc()
so memset is only invoked when CONFIG_SECURITY_NETWORK_XFRM resolves the
stack abuse.
BTW, of the top 10 stack abusers _in the kernel_ (should say based on my
config) constitute 3-4 spots which are caused by this exact thing.
I could send a patch that resolves the issue by moving memset but that
would only fix it for people like myself who turn off SELinux.

> 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.

Yes, that would be cleaner than what i did; i will give the opportunity
to the SELinux folks to take a first crack at it with the above
approach. 

CCing some of the SElinux folks. 
Thanks Dave.

cheers,
jamal

Re: speaking of stacks

[Paul Moore]

On Friday 04 April 2008 8:24:12 am jamal wrote:
> On Thu, 2008-03-04 at 14:18 -0700, David Miller wrote:
> > 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.
>
> Yes, that would be cleaner than what i did; i will give the
> opportunity to the SELinux folks to take a first crack at it with the
> above approach.
>
> CCing some of the SElinux folks.
> Thanks Dave.

Sorry, I've been stuck under a rock for about the past month. Unless 
somebody is really anxious to do this I'll see if I can whip up an RFC 
patch and get it out either today or early next week.

-- 
paul moore
linux @ hp

Re: speaking of stacks

[jamal]

On Fri, 2008-04-04 at 08:32 -0400, Paul Moore wrote:

> Sorry, I've been stuck under a rock for about the past month. Unless 
> somebody is really anxious to do this I'll see if I can whip up an RFC 
> patch and get it out either today or early next week.

No rush - it is an enhancement not a bug fix. If you need my help let me
know.

cheers,
jamal

Re: speaking of stacks

[Paul Moore]

On Friday 04 April 2008 8:41:03 am jamal wrote:
> On Fri, 2008-04-04 at 08:32 -0400, Paul Moore wrote:
> > Sorry, I've been stuck under a rock for about the past month.
> > Unless somebody is really anxious to do this I'll see if I can whip
> > up an RFC patch and get it out either today or early next week.
>
> No rush - it is an enhancement not a bug fix. If you need my help let
> me know.

It would still be nice to get into 2.6.26 and I figure we only have a 
few weeks before the merge window opens.  I've got a patch cobbled 
together that does what Dave suggested plus all the other "left as an 
exercise for the reader" pieces that compiles I just haven't had a 
chance to test it yet, my test machines are busy testing some other 
bits right now.  You should see from mail from me early next week, 
review and additional testing is always welcome :)

-- 
paul moore
linux @ hp