[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: Another slab size-32 leak 2.6.16-rc4-mm2

On Mon, 2006-02-27 at 12:31 -0500, Amy Griffis wrote:
> You are correct that the code as written isn't doing anything other
> than leaking memory.  

Unfortunately, that's right.  The call to audit_ipc_context(ipcp) in
ipc/util.c returns a kmalloc'ed char * that isn't stored anywhere and
quite clearly leaks memory.

I set off on a search through the mailing list posts and my locally
stored patch revisions to determine how something like that slipped

See the post:

On Wed, 19 Oct 2005 16:39:21 -0500, I wrote:
> - This patch reduced some of the verbage required for several functions
> and variables.  In several places, "security_context" was replaced with
> "context", such as
> 	s/audit_ipc_security_context/audit_ipc_context/g
> 	s/audit_log_task_security_context/audit_log_task_context/g
> 	s/audit_aux_data_security_context/audit_aux_data_context/g
> 	s/audit_inode_security_context/audit_inode_context/g
> Additionally, the audit_names data structure uses char *ctx instead of
> char *security_context now (which fits in more with gid,rdev,flags,
> etc).
> Please let me know if anyone is offended by these unrequested changes.
> If this seems good enough to start testing, I'd like to see David merge
> into his try by the end of the week.  Thanks!

A couple of things happened that evidently flew under the radar with no
one screaming until now.
1) audit_ipc_perms() got struct kern_ipc_perm *ipcp added as the last
2) audit_ipc_security_context() was renamed to audit_ipc_context()
3) audit_ipc_context() was changed to return a char * of the context
string, which is called by audit_ipc_perms() (and helped clean up some
of the code in that function)

At this point, I neglected to kill the call to audit_ipc_context() in
ipc/util.c.  Since each call to audit_ipc_perms() was now also passing 
struct kern_ipc_perm *ipcp, the call in ipc/util.c was not needed.

As such, I ack Steve's patch.  It is appropriate to delete the call to
audit_ipc_context() and furthermore make audit_ipc_context() local to

> However, it was intended to collect labels for
> message queues during calls to msgget(), msgrcv(), msgsnd(), etc.  The
> audit_ipc_perms() hook is only collecting labels (and attempted perm
> settings) from IPC_SET operations.

I talked to Klaus about this and I expect him to pipe in right here...

In a nutshell, I was advised back in October that for certification
purposes, we're only required to audit ipc operations involving
security-relevant permissions checks (similar to our certification
requirements on syscall auditing).  This is Klaus's area, though, so
I'll defer to his expertise here.

> This call shouldn't be removed, it should be storing the retrieved
> label in the appropriate place in the audit context.  The label would
> then be freed in audit_free_aux().  

This should be happening in the calls in:
ipc/msg.c, ipc/sem.c, ipc/shm.c:
audit_ipc_perms(setbuf.qbytes, setbuf.uid, setbuf.gid, setbuf.mode,
which calls audit_ipc_context() and gets the security context and stores
it in a (later freed) auxiliary context.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]