On 2018-02-21 19:02, Paul Moore wrote: > On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore <p...@paul-moore.com> wrote: > > On Wed, Feb 21, 2018 at 4:30 AM, Richard Guy Briggs <r...@redhat.com> wrote: > >> If there is a memory allocation error when trying to change an audit > >> kernel feature value, the ignored allocation error will trigger a NULL > >> pointer dereference oops on subsequent use of that pointer. Return > >> instead. > >> > >> Passes audit-testsuite. > >> See: https://github.com/linux-audit/audit-kernel/issues/76 > >> Signed-off-by: Richard Guy Briggs <r...@redhat.com> > >> --- > >> kernel/audit.c | 2 ++ > >> 1 file changed, 2 insertions(+) > > > > Thanks, merged. > > > > In the future a "[PATCH v2]" prefix would be appreciated for patches > > like this, it makes things a little easier in my inbox.
(Sorry, forgot in haste to get that fixed one out...) > After merging this I went through all the other callers to see if they > suffered the same mistake and everyone except for IMA was checking the > returned pointer for NULL. Upon looking at the IMA code, and the > audit code which is called, I realized we are actually "ok" as > audit_log_task_info(), audit_log_format(), audit_log_end(), and others > all check for a NULL audit_buffer at the very top of the functions. > I'm going to leave this patch merged, it's a good practice after all, > but I don't believe that unpatched systems are in any danger of > oops'ing here. On review, agreed. My ctags/cscope DBs need regeneration, so I hadn't noticed that the functions to which I was led weren't the ones I was seeking, and while these three do check, not all functions that accept a struct audit_buffer pointer parameter don't check for NULL. Now that I check, I only find audit_expand (whose callers are all protected) and audit_log_d_path (whose callers all appear to be protected), the latter of which I've spent a bit of time staring at of late (ghak8, ghak21...). We're ok. > >> diff --git a/kernel/audit.c b/kernel/audit.c > >> index 5c25449..2de74be 100644 > >> --- a/kernel/audit.c > >> +++ b/kernel/audit.c > >> @@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 > >> old_feature, u32 new_feature > >> return; > >> > >> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > >> + if (!ab) > >> + return; > >> audit_log_task_info(ab, current); > >> audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u > >> new_lock=%u res=%d", > >> audit_feature_names[which], !!old_feature, > >> !!new_feature, > >> -- > >> 1.8.3.1 > > -- > paul moore > www.paul-moore.com - RGB -- Richard Guy Briggs <r...@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
