PERFORCE change 85491 for review

Robert Watson rwatson at FreeBSD.org
Tue Oct 18 12:34:42 GMT 2005


http://perforce.freebsd.org/chv.cgi?CH=85491

Change 85491 by rwatson at rwatson_zoo on 2005/10/18 12:33:41

	Define ARG_SET_VALID() and ARG_IS_VALID(), which respectively set
	and test kernel record valid bits for various arguments.  Use
	ARG_SET_VALID() when setting flags as a result of collecting an
	argument.  Simplify or clarify some flag-related processing;
	annotate some additional places where simplification or
	clarification is called for but not yet present.

Affected files ...

.. //depot/projects/trustedbsd/audit3/sys/security/audit/audit_arg.c#5 edit
.. //depot/projects/trustedbsd/audit3/sys/security/audit/audit_private.h#9 edit

Differences ...

==== //depot/projects/trustedbsd/audit3/sys/security/audit/audit_arg.c#5 (text+ko) ====

@@ -60,7 +60,7 @@
 		return;
 
 	ar->k_ar.ar_arg_addr = addr;
-	ar->k_ar.ar_valid_arg |= ARG_ADDR;
+	ARG_SET_VALID(ar, ARG_ADDR);
 }
 
 void
@@ -74,7 +74,7 @@
 
 	ar->k_ar.ar_arg_exitstatus = status;
 	ar->k_ar.ar_arg_exitretval = retval;
-	ar->k_ar.ar_valid_arg |= ARG_EXIT;
+	ARG_SET_VALID(ar, ARG_EXIT);
 }
 
 void
@@ -87,7 +87,7 @@
 		return;
 
 	ar->k_ar.ar_arg_len = len;
-	ar->k_ar.ar_valid_arg |= ARG_LEN;
+	ARG_SET_VALID(ar, ARG_LEN);
 }
 
 void
@@ -100,7 +100,7 @@
 		return;
 
 	ar->k_ar.ar_arg_fd = fd;
-	ar->k_ar.ar_valid_arg |= ARG_FD;
+	ARG_SET_VALID(ar, ARG_FD);
 }
 
 void
@@ -113,7 +113,7 @@
 		return;
 
 	ar->k_ar.ar_arg_fflags = fflags;
-	ar->k_ar.ar_valid_arg |= ARG_FFLAGS;
+	ARG_SET_VALID(ar, ARG_FFLAGS);
 }
 
 void
@@ -126,7 +126,7 @@
 		return;
 
 	ar->k_ar.ar_arg_gid = gid;
-	ar->k_ar.ar_valid_arg |= ARG_GID;
+	ARG_SET_VALID(ar, ARG_GID);
 }
 
 void
@@ -139,7 +139,7 @@
 		return;
 
 	ar->k_ar.ar_arg_uid = uid;
-	ar->k_ar.ar_valid_arg |= ARG_UID;
+	ARG_SET_VALID(ar, ARG_UID);
 }
 
 void
@@ -152,7 +152,7 @@
 		return;
 
 	ar->k_ar.ar_arg_egid = egid;
-	ar->k_ar.ar_valid_arg |= ARG_EGID;
+	ARG_SET_VALID(ar, ARG_EGID);
 }
 
 void
@@ -165,7 +165,7 @@
 		return;
 
 	ar->k_ar.ar_arg_euid = euid;
-	ar->k_ar.ar_valid_arg |= ARG_EUID;
+	ARG_SET_VALID(ar, ARG_EUID);
 }
 
 void
@@ -178,7 +178,7 @@
 		return;
 
 	ar->k_ar.ar_arg_rgid = rgid;
-	ar->k_ar.ar_valid_arg |= ARG_RGID;
+	ARG_SET_VALID(ar, ARG_RGID);
 }
 
 void
@@ -191,7 +191,7 @@
 		return;
 
 	ar->k_ar.ar_arg_ruid = ruid;
-	ar->k_ar.ar_valid_arg |= ARG_RUID;
+	ARG_SET_VALID(ar, ARG_RUID);
 }
 
 void
@@ -204,7 +204,7 @@
 		return;
 
 	ar->k_ar.ar_arg_sgid = sgid;
-	ar->k_ar.ar_valid_arg |= ARG_SGID;
+	ARG_SET_VALID(ar, ARG_SGID);
 }
 
 void
@@ -217,7 +217,7 @@
 		return;
 
 	ar->k_ar.ar_arg_suid = suid;
-	ar->k_ar.ar_valid_arg |= ARG_SUID;
+	ARG_SET_VALID(ar, ARG_SUID);
 }
 
 void
@@ -233,7 +233,7 @@
 	for (i = 0; i < gidset_size; i++)
 		ar->k_ar.ar_arg_groups.gidset[i] = gidset[i];
 	ar->k_ar.ar_arg_groups.gidset_size = gidset_size;
-	ar->k_ar.ar_valid_arg |= ARG_GROUPSET;
+	ARG_SET_VALID(ar, ARG_GROUPSET);
 }
 
 void
@@ -245,16 +245,8 @@
 	if (ar == NULL)
 		return;
 
-#if 0
-	/*
-	 * XXX: Add strlcpy() to Darwin for improved safety.
-	 */
 	strlcpy(ar->k_ar.ar_arg_login, login, MAXLOGNAME);
-#else
-	strcpy(ar->k_ar.ar_arg_login, login);
-#endif
-
-	ar->k_ar.ar_valid_arg |= ARG_LOGIN;
+	ARG_SET_VALID(ar, ARG_LOGIN);
 }
 
 void
@@ -268,7 +260,7 @@
 
 	bcopy(name, &ar->k_ar.ar_arg_ctlname, namelen * sizeof(int));
 	ar->k_ar.ar_arg_len = namelen;
-	ar->k_ar.ar_valid_arg |= (ARG_CTLNAME | ARG_LEN);
+	ARG_SET_VALID(ar, ARG_CTLNAME | ARG_LEN);
 }
 
 void
@@ -281,7 +273,7 @@
 		return;
 
 	ar->k_ar.ar_arg_mask = mask;
-	ar->k_ar.ar_valid_arg |= ARG_MASK;
+	ARG_SET_VALID(ar, ARG_MASK);
 }
 
 void
@@ -294,7 +286,7 @@
 		return;
 
 	ar->k_ar.ar_arg_mode = mode;
-	ar->k_ar.ar_valid_arg |= ARG_MODE;
+	ARG_SET_VALID(ar, ARG_MODE);
 }
 
 void
@@ -307,7 +299,7 @@
 		return;
 
 	ar->k_ar.ar_arg_dev = dev;
-	ar->k_ar.ar_valid_arg |= ARG_DEV;
+	ARG_SET_VALID(ar, ARG_DEV);
 }
 
 void
@@ -320,7 +312,7 @@
 		return;
 
 	ar->k_ar.ar_arg_value = value;
-	ar->k_ar.ar_valid_arg |= ARG_VALUE;
+	ARG_SET_VALID(ar, ARG_VALUE);
 }
 
 void
@@ -334,7 +326,7 @@
 
 	ar->k_ar.ar_arg_uid = uid;
 	ar->k_ar.ar_arg_gid = gid;
-	ar->k_ar.ar_valid_arg |= (ARG_UID | ARG_GID);
+	ARG_SET_VALID(ar, ARG_UID | ARG_GID);
 }
 
 void
@@ -347,8 +339,7 @@
 		return;
 
 	ar->k_ar.ar_arg_pid = pid;
-	ar->k_ar.ar_valid_arg |= ARG_PID;
-
+	ARG_SET_VALID(ar, ARG_PID);
 }
 
 void
@@ -369,11 +360,9 @@
 	ar->k_ar.ar_arg_ruid = p->p_ucred->cr_ruid;
 	ar->k_ar.ar_arg_rgid = p->p_ucred->cr_rgid;
 	ar->k_ar.ar_arg_asid = p->p_au->ai_asid;
-
 	ar->k_ar.ar_arg_termid = p->p_au->ai_termid;
-
-	ar->k_ar.ar_valid_arg |= ARG_AUID | ARG_EUID | ARG_EGID | ARG_RUID | 
-		ARG_RGID | ARG_ASID | ARG_TERMID | ARG_PROCESS;
+	ARG_SET_VALID(ar, ARG_AUID | ARG_EUID | ARG_EGID | ARG_RUID |
+	    ARG_RGID | ARG_ASID | ARG_TERMID | ARG_PROCESS);
 }
 
 void
@@ -386,13 +375,12 @@
 		return;
 
 	ar->k_ar.ar_arg_signum = signum;
-	ar->k_ar.ar_valid_arg |= ARG_SIGNUM;
+	ARG_SET_VALID(ar, ARG_SIGNUM);
 }
 
 void
 audit_arg_socket(int sodomain, int sotype, int soprotocol)
 {
-
 	struct kaudit_record *ar;
  
 	ar = currecord();
@@ -402,7 +390,7 @@
 	ar->k_ar.ar_arg_sockinfo.so_domain = sodomain;
 	ar->k_ar.ar_arg_sockinfo.so_type = sotype;
 	ar->k_ar.ar_arg_sockinfo.so_protocol = soprotocol;
-	ar->k_ar.ar_valid_arg |= ARG_SOCKINFO;
+	ARG_SET_VALID(ar, ARG_SOCKINFO);
 }
 
 /*
@@ -421,15 +409,17 @@
 	bcopy(so, &ar->k_ar.ar_arg_sockaddr, sizeof(ar->k_ar.ar_arg_sockaddr));
 	switch (so->sa_family) {
 	case AF_INET:
-		ar->k_ar.ar_valid_arg |= ARG_SADDRINET;
+		ARG_SET_VALID(ar, ARG_SADDRINET);
 		break;
+
 	case AF_INET6:
-		ar->k_ar.ar_valid_arg |= ARG_SADDRINET6;
+		ARG_SET_VALID(ar, ARG_SADDRINET6);
 		break;
+
 	case AF_UNIX:
 		audit_arg_upath(td, ((struct sockaddr_un *)so)->sun_path, 
 				ARG_UPATH1);
-		ar->k_ar.ar_valid_arg |= ARG_SADDRUNIX;
+		ARG_SET_VALID(ar, ARG_SADDRUNIX);
 		break;
 	/* XXXAUDIT: default:? */
 	}
@@ -445,7 +435,7 @@
 		return;
 
 	ar->k_ar.ar_arg_auid = auid;
-	ar->k_ar.ar_valid_arg |= ARG_AUID;
+	ARG_SET_VALID(ar, ARG_AUID);
 }
 
 void
@@ -463,7 +453,7 @@
 	ar->k_ar.ar_arg_amask.am_failure = au_info->ai_mask.am_failure;
 	ar->k_ar.ar_arg_termid.port = au_info->ai_termid.port;
 	ar->k_ar.ar_arg_termid.machine = au_info->ai_termid.machine;
-	ar->k_ar.ar_valid_arg |= ARG_AUID | ARG_ASID | ARG_AMASK | ARG_TERMID;
+	ARG_SET_VALID(ar, ARG_AUID | ARG_ASID | ARG_AMASK | ARG_TERMID);
 }
 
 void
@@ -475,6 +465,9 @@
 	if (ar == NULL)
 		return;
 
+	/*
+	 * XXXAUDIT: Why do we accept a possibly NULL string here?
+	 */
 	/* Invalidate the text string */
 	ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_TEXT);
 	if (text == NULL)
@@ -484,7 +477,7 @@
 		ar->k_ar.ar_arg_text = malloc(MAXPATHLEN, M_AUDIT, M_WAITOK);
 
 	strncpy(ar->k_ar.ar_arg_text, text, MAXPATHLEN);
-	ar->k_ar.ar_valid_arg |= ARG_TEXT;
+	ARG_SET_VALID(ar, ARG_TEXT);
 }
 
 void
@@ -497,7 +490,7 @@
 		return;
 
 	ar->k_ar.ar_arg_cmd = cmd;
-	ar->k_ar.ar_valid_arg |= ARG_CMD;
+	ARG_SET_VALID(ar, ARG_CMD);
 }
 
 void
@@ -510,7 +503,7 @@
 		return;
 
 	ar->k_ar.ar_arg_svipc_cmd = cmd;
-	ar->k_ar.ar_valid_arg |= ARG_SVIPC_CMD;
+	ARG_SET_VALID(ar, ARG_SVIPC_CMD);
 }
 
 void
@@ -524,7 +517,7 @@
 
 	bcopy(perm, &ar->k_ar.ar_arg_svipc_perm, 
 		sizeof(ar->k_ar.ar_arg_svipc_perm));
-	ar->k_ar.ar_valid_arg |= ARG_SVIPC_PERM;
+	ARG_SET_VALID(ar, ARG_SVIPC_PERM);
 }
 
 void
@@ -537,7 +530,7 @@
 		return;
 
 	ar->k_ar.ar_arg_svipc_id = id;
-	ar->k_ar.ar_valid_arg |= ARG_SVIPC_ID;
+	ARG_SET_VALID(ar, ARG_SVIPC_ID);
 }
 
 void
@@ -550,7 +543,7 @@
 		return;
 
 	ar->k_ar.ar_arg_svipc_addr = addr;
-	ar->k_ar.ar_valid_arg |= ARG_SVIPC_ADDR;
+	ARG_SET_VALID(ar, ARG_SVIPC_ADDR);
 }
 
 void
@@ -565,7 +558,7 @@
 	ar->k_ar.ar_arg_pipc_perm.pipc_uid = uid;
 	ar->k_ar.ar_arg_pipc_perm.pipc_gid = gid;
 	ar->k_ar.ar_arg_pipc_perm.pipc_mode = mode;
-	ar->k_ar.ar_valid_arg |= ARG_POSIX_IPC_PERM;
+	ARG_SET_VALID(ar, ARG_POSIX_IPC_PERM);
 }
 
 void
@@ -579,7 +572,7 @@
 
 	bcopy((void *)udata, &ar->k_ar.ar_arg_auditon, 
 		sizeof(ar->k_ar.ar_arg_auditon));
-	ar->k_ar.ar_valid_arg |= ARG_AUDITON;
+	ARG_SET_VALID(ar, ARG_AUDITON);
 }
 
 /*
@@ -601,6 +594,9 @@
 	switch (fp->f_type) {
 	case DTYPE_VNODE:
 	case DTYPE_FIFO:
+		/*
+		 * XXXAUDIT: Only possibly to record as first vnode?
+		 */
 		vp = fp->f_vnode;
 		vfslocked = VFS_LOCK_GIANT(vp->v_mount);
 		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, curthread);
@@ -636,7 +632,7 @@
 				pcb->inp_fport;
 			ar->k_ar.ar_arg_sockinfo.so_lport =
 				pcb->inp_lport;
-			ar->k_ar.ar_valid_arg |= ARG_SOCKINFO;
+			ARG_SET_VALID(ar, ARG_SOCKINFO);
 		}
 		break;
 
@@ -656,7 +652,7 @@
  * XXXAUDIT: Possibly assert that the memory isn't already allocated?
  */
 void
-audit_arg_upath(struct thread *td, char *upath, u_int64_t flags)
+audit_arg_upath(struct thread *td, char *upath, u_int64_t flag)
 {
 	struct kaudit_record *ar;
 	char **pathp;
@@ -667,33 +663,26 @@
 	/*
 	 * XXXAUDIT: Witness warning for possible sleep here?
 	 */
+	KASSERT((flag == ARG_UPATH1) || (flag == ARG_UPATH2),
+	    ("audit_arg_upath: flag %llu", flag));
+	KASSERT((flag != ARG_UPATH1) || (flag != ARG_UPATH2),
+	    ("audit_arg_upath: flag %llu", flag));
 
-	/*
-	 * XXXAUDIT: KASSERT argument validity?
-	 */
-	if ((flags & (ARG_UPATH1 | ARG_UPATH2)) == 0)
-		return;
-
 	ar = currecord();
 	if (ar == NULL)
 		return;
 
-	if (flags & ARG_UPATH1) {
-		ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_UPATH1);
+	if (flag == ARG_UPATH1)
 		pathp = &ar->k_ar.ar_arg_upath1;
-	} else {
-		ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_UPATH2);
+	else
 		pathp = &ar->k_ar.ar_arg_upath2;
-	}
 
 	if (*pathp == NULL)
 		*pathp = malloc(MAXPATHLEN, M_AUDIT, M_WAITOK);
 
 	canon_path(td, upath, *pathp);
-	if (flags & ARG_UPATH1)
-		ar->k_ar.ar_valid_arg |= ARG_UPATH1;
-	else
-		ar->k_ar.ar_valid_arg |= ARG_UPATH2;
+
+	ARG_SET_VALID(ar, flag);
 }
 
 /*
@@ -741,6 +730,10 @@
 
 	/*
 	 * XXXAUDIT: KASSERT argument validity instead?
+	 *
+	 * XXXAUDIT: The below clears, and then resets the flags for valid
+	 * arguments.  Ideally, either the new path is used, or the old one
+	 * would be.
 	 */
 	if ((flags & (ARG_VNODE1 | ARG_VNODE2)) == 0)
 		return;
@@ -796,10 +789,9 @@
 	vnp->vn_fileid = vattr.va_fileid;
 	vnp->vn_gen = vattr.va_gen;
 	if (flags & ARG_VNODE1)
-		ar->k_ar.ar_valid_arg |= ARG_VNODE1;
+		ARG_SET_VALID(ar, ARG_VNODE1);
 	else
-		ar->k_ar.ar_valid_arg |= ARG_VNODE2;
-
+		ARG_SET_VALID(ar, ARG_VNODE2);
 }
 
 /*

==== //depot/projects/trustedbsd/audit3/sys/security/audit/audit_private.h#9 (text+ko) ====

@@ -188,6 +188,16 @@
 };
 
 /*
+ * Arguments in the audit record are initially not defined; flags are set to
+ * indicate if they are present so they can be included in the audit log
+ * stream only if defined.
+ */
+#define	ARG_IS_VALID(kar, arg)	((kar)->k_ar.ar_valid_arg & (arg))
+#define	ARG_SET_VALID(kar, arg) do {					\
+	(kar)->k_ar.ar_valid_arg |= (arg);				\
+} while (0)
+
+/*
  * In-kernel version of audit record; the basic record plus queue meta-data.
  * This record can also have a pointer set to some opaque data that will
  * be passed through to the audit writing mechanism.
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message



More information about the trustedbsd-cvs mailing list