linux-kernel - Re: [PATCH 8/8] audit: add audit_backlog_wait

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: <20130918204949.GI13968@madcap2.tricolour.ca>
Date:	Wed, 18 Sep 2013 16:49:49 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	linux-audit@...hat.com, linux-kernel@...r.kernel.org
Cc:	Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>,
	Konstantin Khlebnikov <khlebnikov@...nvz.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Duval <dan.duval@...cle.com>,
	Chuck Anderson <chuck.anderson@...cle.com>,
	Guy Streeter <streeter@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH 8/8] audit: add audit_backlog_wait_time configuration
 option

On Wed, Sep 18, 2013 at 04:33:25PM -0400, Eric Paris wrote:
> On Wed, 2013-09-18 at 15:06 -0400, Richard Guy Briggs wrote:
> > reaahead-collector abuses the audit logging facility to discover which files
> > are accessed at boot time to make a pre-load list
> > 
> > Add a tuning option to audit_backlog_wait_time so that if auditd can't keep up,
> > or gets blocked, the callers won't be blocked.

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 3d17670..fc535b6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -701,8 +708,21 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >  			if (err < 0)
> >  				return err;
> >  		}
> > -		if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
> > +		if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT) {
> >  			err = audit_set_backlog_limit(s.backlog_limit);
> > +			if (err < 0)
> > +				return err;
> > +		}
> > +		if (s.mask & AUDIT_STATUS_BACKLOG_WAIT_TIME) {
> > +			if (sizeof(s) > (size_t)nlh->nlmsg_len)
> > +				break;
> 
> What gets returned here?  I think err has a value of 0, but it doesn't
> seem to have been clearly intentional.  If they know about the
> AUDIT_STATUS_BACKLOG_WAIT_TIME flag, but they didn't send a long enough
> skb?  That seems like an error condition....

The intent was that it is a NOP, since err would have a value of zero,
but I see your point, that if that flag is present, the struct member
should be too.  My original intent was that if the structure member
wasn't present, it would default to zero, unintentionally setting the
wait time to zero.  It was part of my paranoia in the absence of an API
version indicator.  No harm done, but I agree it should return an error.

Thanks for the catch.

> > +			if (s.backlog_wait_time < 0 ||
> > +			    s.backlog_wait_time > 10*AUDIT_BACKLOG_WAIT_TIME)
> > +				return -EINVAL;

I assume values less than zero or larger than 10 times the current
default of one minute are errors or unreasonable.

Any argument for more than 10 minutes?


- RGB

--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
--
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/