[RFC][PATCH] (#7U3) [linux-2.6.12-rc2-mm1] file system auditing

[Steve Grubb]

On Wednesday 27 April 2005 19:17, Timothy R. Chavez wrote:
> diff -Nurp linux-2.6.12-rc2-mm1~orig/include/linux/audit.h
> linux-2.6.12-rc2-mm1~audit/include/linux/audit.h ---
> linux-2.6.12-rc2-mm1~orig/include/linux/audit.h     2005-04-11
> 09:15:03.000000000 -0500 +++
> linux-2.6.12-rc2-mm1~audit/include/linux/audit.h    2005-04-27
> 16:37:54.000000000 -0500 @@ -24,18 +24,27 @@
>  #ifndef _LINUX_AUDIT_H_
>  #define _LINUX_AUDIT_H_
>  
> +#ifdef __KERNEL__
>  #include <linux/sched.h>
>  #include <linux/elf.h>
>  
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> +#include <asm/atomic.h>
> +#endif

Do you need these headers or just the forward declaration for the data types 
used in this header?

>+struct audit_data {
>+       struct audit_wentry     *wentry;
>+       struct hlist_head       watchlist;
>+       rwlock_t                lock;
>+};

audit_data seems very generic. Like something the core auditing system would 
have. It would be more meaningful if it were named something related to the 
file system or watch. audit_wdata?

>@@ -190,6 +236,7 @@ extern void audit_get_stamp(struct audit
 >extern int  audit_set_loginuid(struct audit_context *ctx, uid_t loginuid);
 >extern uid_t audit_get_loginuid(struct audit_context *ctx);
 >extern int audit_ipc_perms(unsigned long qbytes, uid_t uid, gid_t gid, 
mode_t mode);
>+extern int audit_notify_watch(struct inode *inode, int mask);
 >#else
 >#define audit_alloc(t) ({ 0; })
 >#define audit_free(t) do { ; } while (0)

I wonder if this function declaration should have a #ifdef 
CONFIG_AUDITFILESYSTEM around it and then use the macro if not. Same goes for 
the function definition in auditsc.c.

(audit.c)
>@@ -416,6 +419,17 @@ static int audit_receive_msg(struct sk_b
>                err = -EOPNOTSUPP;
> #endif
>                break;
>+       case AUDIT_WATCH_LIST:
>+               err = audit_list_watches(pid, seq);
>+               break;
>+       case AUDIT_WATCH_INS:
>+       case AUDIT_WATCH_REM:
>+               if (nlh->nlmsg_len < sizeof(struct audit_transport))
>+                       return -EINVAL;
>+               err = audit_receive_watch(nlh->nlmsg_type,
>+                                         NETLINK_CB(skb).pid,
                       
Should the call to audit_list_watches be surrounded by #ifdef 
CONFIG_AUDITFILESYSTEM  and then return -EOPNOTSUPP in the #else? Same with 
audit_receive_watch.

Looking through the code for auditfs.c, there is no use of the macros likely 
and unlikely to help branch prediction. I'll throw this out to Chris & David, 
do we have any critical paths that would benefit from this code being 
augmented with those macros?

(auditfs.c)
>+       /* Payload */
>+       memcpy(memblk, &req, sizeof(req));
>+       offset = total - req.fklen;
>+       memcpy(memblk + offset, watch->filterkey, req.fklen);
>+       offset = offset - req.pathlen;
>+       memcpy(memblk + offset, watch->path, req.pathlen);

Are these offsets being calculated right?
Seems like sb 

+       memcpy(memblk, &req, sizeof(req));
+       offset = sizeof(req);
+       memcpy(memblk + offset, watch->filterkey, req.fklen);
+       offset += req.fklen;
+       memcpy(memblk + offset, watch->path, req.pathlen)

?

>+int audit_receive_watch(int type, int pid, int uid, int seq,
>+                       struct audit_transport *req)
>+{
>+       int ret = 0;
>+       char *path = NULL;
>+       char *filterkey = NULL;
>+
>+       ret = -ENOMEM;
>+       path = kmalloc(req->pathlen, GFP_KERNEL);
>+       if (!path)
>+               goto audit_receive_watch_exit;
>+       strncpy(path, req->buf, req->pathlen);
>+
>+       if (req->fklen) {
>+               filterkey = kmalloc(req->fklen, GFP_KERNEL);
>+               if (!filterkey)
>+                       goto audit_receive_watch_exit;
>+       }
>+       strncpy(filterkey, req->buf+req->pathlen, req->fklen);
>+
>+       ret = -EINVAL;
>+       if (!path || strlen(path) + 1 > PATH_MAX)
>+               goto audit_receive_watch_exit;
>+
>+       /* Includes terminating '\0' */
>+       if (req->fklen > AUDIT_FILTERKEY_MAX)
>+               goto audit_receive_watch_exit;
>+
>+       if (req->perms > (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND))
>+               goto audit_receive_watch_exit;

Whatever happened to checking parameters and then allocating memory? I thought 
this was done right a few revs ago.

>+int audit_notify_watch(struct inode *inode, int mask)
>+{
>+       int ret = 0;
>+       struct audit_context *context = current->audit_context;
>+       struct audit_aux_data_watched *ax;
>+       struct audit_wentry *wentry = NULL;
>+
>+       if (likely(!context))
>+               goto audit_notify_watch_fail;

Question. If there is no context, is there a possibility of generating one? 
Under what circumstances besides no memory would a context not exist?

Based on what I see, I think I can merge the userspace portion for listing 
watches. There's cleanups needed, but I think the intent is correct.

-Steve