LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Smalley <sds@tycho.nsa.gov>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Al Viro <viro@ftp.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Steve Grubb <sgrubb@redhat.com>,
	James Morris <jmorris@namei.org>
Subject: Re: [GIT PULL] audit patches
Date: Thu, 22 Feb 2007 08:22:47 -0500	[thread overview]
Message-ID: <1172150567.14363.337.camel@moss-spartans.epoch.ncsc.mil> (raw)
In-Reply-To: <20070221160319.6de08b64.akpm@linux-foundation.org>

On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> On Sun, 18 Feb 2007 04:01:27 +0000 Al Viro <viro@ftp.linux.org.uk> wrote:
> 
> > Misc audit patches (resend again...); the most intrusive one is AUDIT_FD_PAIR,
> > allowing to log descriptor numbers from syscalls that do not return them in
> > usual way (i.e. pipe() and socketpair()).  It took some massage of
> > the failure exits in sys_socketpair(); the rest is absolutely trivial.
> > Please, pull from
> > git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/ audit.b37
> 
> Please send patches to the list for review if practical?  In this case it
> was.  I trust davem has had a look at the non-trivial changes to
> sys_socketpair().
> 
> 
> 
> Looking at the changes to audit_receive_msg():
> 
> 
> 				if (sid) {
> 					if (selinux_sid_to_string(
> 							sid, &ctx, &len)) {
> 						audit_log_format(ab,
> 							" ssid=%u", sid);
> 						/* Maybe call audit_panic? */
> 					} else
> 						audit_log_format(ab,
> 							" subj=%s", ctx);
> 					kfree(ctx);
> 				}
> 
> This is assuming that selinux_sid_to_string() always initialises `ctx'.
> 
> But AFAICT there are two error paths in security_sid_to_context() which
> forget to do that, so we end up doing kfree(uninitialised-local).
> 
> I'd consider that a shortcoming in security_sid_to_context(), so not a
> problem in this patch, as long as people agree with my blaming above.

I wouldn't assume that the function initializes an argument if it
returns an error, and at least some of the callers (in auditsc.c) appear
to correctly initialize ctx to NULL themselves before calling
selinux_sid_to_string().  But if you'd prefer the function to always
handle it, we can do that.

> 
> The coding style in there is a bit odd-looking.
> 
> The new __audit_fd_pair() has unneeded braces in it.
-- 
Stephen Smalley
National Security Agency


  reply	other threads:[~2007-02-22 13:27 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-18  4:01 Al Viro
2007-02-22  0:03 ` Andrew Morton
2007-02-22 13:22   ` Stephen Smalley [this message]
2007-02-22 21:19     ` Andrew Morton
2007-02-23 14:00       ` [patch 1/1] selinux: always initialize arguments to security_sid_to_context (Was: Re: [GIT PULL] audit patches) Stephen Smalley
2008-04-29 18:08 [git pull] audit patches Al Viro
2009-04-05 19:17 Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1172150567.14363.337.camel@moss-spartans.epoch.ncsc.mil \
    --to=sds@tycho.nsa.gov \
    --cc=akpm@linux-foundation.org \
    --cc=jmorris@namei.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sgrubb@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ftp.linux.org.uk \
    --subject='Re: [GIT PULL] audit patches' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).