linux-kernel - Re: [PATCH V6 4/4] audit: avoid double copying the audit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1437102090.18955.93.camel@redhat.com>
Date:	Thu, 16 Jul 2015 22:01:30 -0500
From:	Eric Paris <eparis@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>, Paul Moore <pmoore@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com, pmoody@...gle.com
Subject: Re: [PATCH V6 4/4] audit: avoid double copying the audit_exe path
 string

I have to admit, I'm partial to not merging this (with the other
patches).  Changing object lifetimes in what i seem to remember is long
standing code (auditfilter, not auditexe) seems to me like something we
really would want to be git bisectable, not mushed with an unrelated
feature addition. But it ain't my tree   :)

-Eric

On Thu, 2015-07-16 at 22:01 -0400, Richard Guy Briggs wrote:
> On 15/07/16, Paul Moore wrote:
> > On Tuesday, July 14, 2015 11:50:26 AM Richard Guy Briggs wrote:
> > > Make this interface consistent with watch and filter key, 
> > > avoiding the extra
> > > string copy and simply consume the new string pointer.
> > > 
> > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > > ---
> > >  kernel/audit_exe.c      |    8 ++++++--
> > >  kernel/audit_fsnotify.c |    9 +--------
> > >  kernel/auditfilter.c    |    2 +-
> > >  3 files changed, 8 insertions(+), 11 deletions(-)
> > 
> > Merge this patch too, there is no reason why these needs to be its 
> > own patch.
> 
> I wanted to keep this patch seperate until it is well understood and
> accepted rather than mix it in.
> 
> I'm fine merging it if you prefer.
> 
> > > diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> > > index 75ad4f2..09e4eb4 100644
> > > --- a/kernel/audit_exe.c
> > > +++ b/kernel/audit_exe.c
> > > @@ -27,11 +27,15 @@ int audit_dupe_exe(struct audit_krule *new, 
> > > struct
> > > audit_krule *old) struct audit_fsnotify_mark *audit_mark;
> > >  	char *pathname;
> > > 
> > > -	pathname = audit_mark_path(old->exe);
> > > +	pathname = kstrdup(audit_mark_path(old->exe), 
> > > GFP_KERNEL);
> > > +	if (!pathname)
> > > +		return -ENOMEM;
> > > 
> > >  	audit_mark = audit_alloc_mark(new, pathname, 
> > > strlen(pathname));
> > > -	if (IS_ERR(audit_mark))
> > > +	if (IS_ERR(audit_mark)) {
> > > +		kfree(pathname);
> > >  		return PTR_ERR(audit_mark);
> > > +	}
> > >  	new->exe = audit_mark;
> > > 
> > >  	return 0;
> > > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> > > index a4e7b16..e57e08a 100644
> > > --- a/kernel/audit_fsnotify.c
> > > +++ b/kernel/audit_fsnotify.c
> > > @@ -94,7 +94,6 @@ struct audit_fsnotify_mark 
> > > *audit_alloc_mark(struct
> > > audit_krule *krule, char *pa struct dentry *dentry;
> > >  	struct inode *inode;
> > >  	unsigned long ino;
> > > -	char *local_pathname;
> > >  	dev_t dev;
> > >  	int ret;
> > > 
> > > @@ -115,21 +114,15 @@ struct audit_fsnotify_mark 
> > > *audit_alloc_mark(struct
> > > audit_krule *krule, char *pa ino = dentry->d_inode->i_ino;
> > >  	}
> > > 
> > > -	audit_mark = ERR_PTR(-ENOMEM);
> > > -	local_pathname = kstrdup(pathname, GFP_KERNEL);
> > > -	if (!local_pathname)
> > > -		goto out;
> > > -
> > >  	audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
> > >  	if (unlikely(!audit_mark)) {
> > > -		kfree(local_pathname);
> > >  		audit_mark = ERR_PTR(-ENOMEM);
> > >  		goto out;
> > >  	}
> > > 
> > >  	fsnotify_init_mark(&audit_mark->mark, 
> > > audit_fsnotify_free_mark);
> > >  	audit_mark->mark.mask = AUDIT_FS_EVENTS;
> > > -	audit_mark->path = local_pathname;
> > > +	audit_mark->path = pathname;
> > >  	audit_mark->ino = ino;
> > >  	audit_mark->dev = dev;
> > >  	audit_mark->rule = krule;
> > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > index f65c97f..f46ed69 100644
> > > --- a/kernel/auditfilter.c
> > > +++ b/kernel/auditfilter.c
> > > @@ -559,8 +559,8 @@ static struct audit_entry 
> > > *audit_data_to_entry(struct
> > > audit_rule_data *data, entry->rule.buflen += f->val;
> > > 
> > >  			audit_mark = audit_alloc_mark(&entry
> > > ->rule, str, f->val);
> > > -			kfree(str);
> > >  			if (IS_ERR(audit_mark)) {
> > > +				kfree(str);
> > >  				err = PTR_ERR(audit_mark);
> > >  				goto exit_free;
> > >  			}
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/