Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <6effbbd4574407d6af21162e57d9102d5f8b02ed.1594664015.git.rgb@redhat.com>
 <CAHC9VhSyq7yKQqwvHL5syU9+TFki6-__WfCrvqewbnU3xpND4Q@mail.gmail.com>
 <20200714174353.ds7lj3iisy67t2zu@madcap2.tricolour.ca> <CAHC9VhQusQsdQc7EfdjdH5mp6qqqYVPHnG9nNhUhf3DS_cdWwA@mail.gmail.com>
 <20200714210027.me2ieywjfcsf4v5r@madcap2.tricolour.ca> <CAHC9VhQgDGPutYxQawMPmezm1a+i1nXO5KSn9_7KPDZsRBJ4pw@mail.gmail.com>
 <e6eb37d5-ec6b-852a-74df-bbf453607fbe@canonical.com>
In-Reply-To: <e6eb37d5-ec6b-852a-74df-bbf453607fbe@canonical.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 21 Jul 2020 17:16:24 -0400
Message-ID: <CAHC9VhSoUBqXh7ikVdpr9-e2+3Wx-A05g5EBjD3ka1i1xF2vMg@mail.gmail.com>
Subject: Re: [PATCH ghak84 v4] audit: purge audit_log_string from the
 intra-kernel audit API
To:     John Johansen <john.johansen@canonical.com>
Cc:     Richard Guy Briggs <rgb@redhat.com>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Security Module list 
        <linux-security-module@vger.kernel.org>,
        Eric Paris <eparis@parisplace.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 21, 2020 at 3:31 PM John Johansen
<john.johansen@canonical.com> wrote:
> On 7/21/20 8:19 AM, Paul Moore wrote:
> > On Tue, Jul 14, 2020 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >> On 2020-07-14 16:29, Paul Moore wrote:
> >>> On Tue, Jul 14, 2020 at 1:44 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>>> On 2020-07-14 12:21, Paul Moore wrote:
> >>>>> On Mon, Jul 13, 2020 at 3:52 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>>>>>
> >>>>>> audit_log_string() was inteded to be an internal audit function and
> >>>>>> since there are only two internal uses, remove them.  Purge all external
> >>>>>> uses of it by restructuring code to use an existing audit_log_format()
> >>>>>> or using audit_log_format().
> >>>>>>
> >>>>>> Please see the upstream issue
> >>>>>> https://github.com/linux-audit/audit-kernel/issues/84
> >>>>>>
> >>>>>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >>>>>> ---
> >>>>>> Passes audit-testsuite.
> >>>>>>
> >>>>>> Changelog:
> >>>>>> v4
> >>>>>> - use double quotes in all replaced audit_log_string() calls
> >>>>>>
> >>>>>> v3
> >>>>>> - fix two warning: non-void function does not return a value in all control paths
> >>>>>>         Reported-by: kernel test robot <lkp@intel.com>
> >>>>>>
> >>>>>> v2
> >>>>>> - restructure to piggyback on existing audit_log_format() calls, checking quoting needs for each.
> >>>>>>
> >>>>>> v1 Vlad Dronov
> >>>>>> - https://github.com/nefigtut/audit-kernel/commit/dbbcba46335a002f44b05874153a85b9cc18aebf
> >>>>>>
> >>>>>>  include/linux/audit.h     |  5 -----
> >>>>>>  kernel/audit.c            |  4 ++--
> >>>>>>  security/apparmor/audit.c | 10 ++++------
> >>>>>>  security/apparmor/file.c  | 25 +++++++------------------
> >>>>>>  security/apparmor/ipc.c   | 46 +++++++++++++++++++++++-----------------------
> >>>>>>  security/apparmor/net.c   | 14 ++++++++------
> >>>>>>  security/lsm_audit.c      |  4 ++--
> >>>>>>  7 files changed, 46 insertions(+), 62 deletions(-)
> >>>>>
> >>>>> Thanks for restoring the quotes, just one question below ...
> >>>>>
> >>>>>> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> >>>>>> index 4ecedffbdd33..fe36d112aad9 100644
> >>>>>> --- a/security/apparmor/ipc.c
> >>>>>> +++ b/security/apparmor/ipc.c
> >>>>>> @@ -20,25 +20,23 @@
> >>>>>>
> >>>>>>  /**
> >>>>>>   * audit_ptrace_mask - convert mask to permission string
> >>>>>> - * @buffer: buffer to write string to (NOT NULL)
> >>>>>>   * @mask: permission mask to convert
> >>>>>> + *
> >>>>>> + * Returns: pointer to static string
> >>>>>>   */
> >>>>>> -static void audit_ptrace_mask(struct audit_buffer *ab, u32 mask)
> >>>>>> +static const char *audit_ptrace_mask(u32 mask)
> >>>>>>  {
> >>>>>>         switch (mask) {
> >>>>>>         case MAY_READ:
> >>>>>> -               audit_log_string(ab, "read");
> >>>>>> -               break;
> >>>>>> +               return "read";
> >>>>>>         case MAY_WRITE:
> >>>>>> -               audit_log_string(ab, "trace");
> >>>>>> -               break;
> >>>>>> +               return "trace";
> >>>>>>         case AA_MAY_BE_READ:
> >>>>>> -               audit_log_string(ab, "readby");
> >>>>>> -               break;
> >>>>>> +               return "readby";
> >>>>>>         case AA_MAY_BE_TRACED:
> >>>>>> -               audit_log_string(ab, "tracedby");
> >>>>>> -               break;
> >>>>>> +               return "tracedby";
> >>>>>>         }
> >>>>>> +       return "";
> >>>>>
> >>>>> Are we okay with this returning an empty string ("") in this case?
> >>>>> Should it be a question mark ("?")?
> >>>>>
> >>>>> My guess is that userspace parsing should be okay since it still has
> >>>>> quotes, I'm just not sure if we wanted to use a question mark as we do
> >>>>> in other cases where the field value is empty/unknown.
> >>>>
> >>>> Previously, it would have been an empty value, not even double quotes.
> >>>> "?" might be an improvement.
> >>>
> >>> Did you want to fix that now in this patch, or leave it to later?  As
> >>> I said above, I'm not too bothered by it with the quotes so either way
> >>> is fine by me.
> >>
> >> I'd defer to Steve, otherwise I'd say leave it, since there wasn't
> >> anything there before and this makes that more evident.
> >>
> >>> John, I'm assuming you are okay with this patch?
> >
> > With no comments from John or Steve in the past week, I've gone ahead
> > and merged the patch into audit/next.
>
> sorry, for some reason I thought a new iteration of this was coming.
>
> the patch is fine, the empty unknown value should be possible here
> so changing it to "?" won't affect anything.

Yeah, I was kind of on the fence about requiring a new version from
Richard.  I think "?" is arguably the right approach, but I don't
think it matters enough to force the issue.  If it proves to be
problematic we can fix it later.

Regardless, it's in audit/next now.

-- 
paul moore
www.paul-moore.com