speaking of stacks

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

From: jamal <[email protected]>
Date: Mon, 31 Mar 2008 09:15:56 -0400

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.

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.

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

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

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

cheers,
jamal

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