[PATCH 00/10] LSM documentation update

[PATCH 00/10] LSM documentation update

[Denis Efremov]

Recent "New LSM Hooks" discussion has led me to the
thought that it might be a good idea to slightly
update the current documentation. The patchset adds
nothing new to the documentation, only fixes the old
description of hooks to reflect their current state.

Denis Efremov (10):
  security: fix documentation for the sb_copy_data hook
  security: fix documentation for the syslog hook
  security: fix documentation for the socket_post_create hook
  security: fix documentation for the task_setscheduler hook
  security: fix documentation for the socket_getpeersec_dgram hook
  security: fix documentation for the path_chmod hook
  security: fix documentation for the audit_* hooks
  security: fix documentation for the msg_queue_* hooks
  security: fix documentation for the sem_* hooks
  security: fix documentation for the shm_* hooks

 include/linux/lsm_hooks.h | 127 ++++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 66 deletions(-)

-- 
2.17.2

[PATCH 01/10] security: fix documentation for the sb_copy_data hook

[Denis Efremov]

The @type argument of the sb_copy_data hook was removed
in the commit "LSM/SELinux: Interfaces to allow FS to control
mount options" (e0007529893c). This commit removes the description
of the @type argument from the LSM documentation.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9a0bdf91e646..de179331be5c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -111,7 +111,6 @@
  *	options cleanly (a filesystem may modify the data e.g. with strsep()).
  *	This also allows the original mount data to be stripped of security-
  *	specific options to avoid having to make filesystems aware of them.
- *	@type the type of filesystem being mounted.
  *	@orig the original mount data copied from userspace.
  *	@copy copied data which will be passed to the security module.
  *	Returns 0 if the copy was successful.
-- 
2.17.2

[PATCH 02/10] security: fix documentation for the syslog hook

[Denis Efremov]

The syslog hook was changed in the commit
"capabilities/syslog: open code cap_syslog logic to
fix build failure" (12b3052c3ee8). The argument @from_file
was removed from the hook. This patch updates the
documentation for the syslog hook accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index de179331be5c..a0555683db63 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1276,7 +1276,6 @@
  *	logging to the console.
  *	See the syslog(2) manual page for an explanation of the @type values.
  *	@type contains the type of action.
- *	@from_file indicates the context of action (if it came from /proc).
  *	Return 0 if permission is granted.
  * @settime:
  *	Check permission to change the system time.
-- 
2.17.2

[PATCH 03/10] security: fix documentation for the socket_post_create hook

[Denis Efremov]

This patch slightly fixes the documentation for the
socket_post_create hook. The documentation states that
i_security field is accessible through inode field of socket
structure (i.e., 'sock->inode->i_security'). There is no inode
field in the socket structure. The i_security field is accessible
through SOCK_INODE macro. The patch updates the documentation
to reflect this.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a0555683db63..80e5f3421b91 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -752,9 +752,9 @@
  *	socket structure, but rather, the socket security information is stored
  *	in the associated inode.  Typically, the inode alloc_security hook will
  *	allocate and and attach security information to
- *	sock->inode->i_security.  This hook may be used to update the
- *	sock->inode->i_security field with additional information that wasn't
- *	available when the inode was allocated.
+ *	SOCK_INODE(sock)->i_security.  This hook may be used to update the
+ *	SOCK_INODE(sock)->i_security field with additional information that
+ *	wasn't available when the inode was allocated.
  *	@sock contains the newly created socket structure.
  *	@family contains the requested protocol family.
  *	@type contains the requested communications type.
-- 
2.17.2

[PATCH 04/10] security: fix documentation for the task_setscheduler hook

[Denis Efremov]

The task_setscheduler hook was changed in the commit
"security: remove unused parameter from security_task_setscheduler()"
(b0ae19811375). The arguments @policy, @lp were removed from the hook.
This patch updates the documentation accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 80e5f3421b91..09223911876e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -655,10 +655,8 @@
  *	Return 0 if permission is granted.
  * @task_setscheduler:
  *	Check permission before setting scheduling policy and/or parameters of
- *	process @p based on @policy and @lp.
+ *	process @p.
  *	@p contains the task_struct for process.
- *	@policy contains the scheduling policy.
- *	@lp contains the scheduling parameters.
  *	Return 0 if permission is granted.
  * @task_getscheduler:
  *	Check permission before obtaining scheduling information for process
-- 
2.17.2

[PATCH 05/10] security: fix documentation for the socket_getpeersec_dgram hook

[Denis Efremov]

The socket_getpeersec_dgram hook was changed in the commit
"[AF_UNIX]: Kernel memory leak fix for af_unix datagram
getpeersec patch" (dc49c1f94e34). The arguments @secdata
and @seclen were changed to @sock and @secid. This patch
updates the documentation accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 09223911876e..cb93972257be 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -861,9 +861,9 @@
  *	the IP_PASSSEC option via getsockopt.  It can then retrieve the
  *	security state returned by this hook for a packet via the SCM_SECURITY
  *	ancillary message type.
- *	@skb is the skbuff for the packet being queried
- *	@secdata is a pointer to a buffer in which to copy the security data
- *	@seclen is the maximum length for @secdata
+ *	@sock contains the socket structure.
+ *	@skb is the skbuff for the packet being queried.
+ *	@secid pointer to store the secid of the packet.
  *	Return 0 on success, error on failure.
  * @sk_alloc_security:
  *	Allocate and attach a security structure to the sk->sk_security field,
-- 
2.17.2

[PATCH 06/10] security: fix documentation for the path_chmod hook

[Denis Efremov]

The path_chmod hook was changed in the commit
"switch security_path_chmod() to struct path *" (cdcf116d44e7).
The argument @mnt was removed from the hook, @dentry was changed
to @path. This patch updates the documentation accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index cb93972257be..5d6428d0027b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -304,8 +304,7 @@
  *	Return 0 if permission is granted.
  * @path_chmod:
  *	Check for permission to change DAC's permission of a file or directory.
- *	@dentry contains the dentry structure.
- *	@mnt contains the vfsmnt structure.
+ *	@path contains the path structure.
  *	@mode contains DAC's mode.
  *	Return 0 if permission is granted.
  * @path_chown:
-- 
2.17.2

[PATCH 07/10] security: fix documentation for the audit_* hooks

[Denis Efremov]

This patch updates the documentation for the audit_* hooks
to use the same arguments names as in the hook's declarations.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5d6428d0027b..307714b8b072 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1329,7 +1329,7 @@
  * @audit_rule_known:
  *	Specifies whether given @rule contains any fields related to
  *	current LSM.
- *	@rule contains the audit rule of interest.
+ *	@krule contains the audit rule of interest.
  *	Return 1 in case of relation found, 0 otherwise.
  *
  * @audit_rule_match:
@@ -1338,14 +1338,14 @@
  *	@secid contains the security id in question.
  *	@field contains the field which relates to current LSM.
  *	@op contains the operator that will be used for matching.
- *	@rule points to the audit rule that will be checked against.
+ *	@lsmrule points to the audit rule that will be checked against.
  *	@actx points to the audit context associated with the check.
  *	Return 1 if secid matches the rule, 0 if it does not, -ERRNO on failure.
  *
  * @audit_rule_free:
  *	Deallocate the LSM audit rule structure previously allocated by
  *	audit_rule_init.
- *	@rule contains the allocated rule
+ *	@lsmrule contains the allocated rule.
  *
  * @inode_invalidate_secctx:
  *	Notify the security module that it must revalidate the security context
-- 
2.17.2

[PATCH 08/10] security: fix documentation for the msg_queue_* hooks

[Denis Efremov]

The msg_queue_* hooks were changed in the commit
"msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue
security hooks" (d8c6e8543294). The type of the argument msq was changed
from msq_queue to kern_ipc_perm. This patch updates the documentation
for the hooks accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 307714b8b072..f61f39c73208 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1093,41 +1093,41 @@
  *
  * @msg_queue_alloc_security:
  *	Allocate and attach a security structure to the
- *	msq->q_perm.security field. The security field is initialized to
+ *	perm->security field. The security field is initialized to
  *	NULL when the structure is first created.
- *	@msq contains the message queue structure to be modified.
+ *	@perm contains the IPC permissions of the message queue.
  *	Return 0 if operation was successful and permission is granted.
  * @msg_queue_free_security:
  *	Deallocate security structure for this message queue.
- *	@msq contains the message queue structure to be modified.
+ *	@perm contains the IPC permissions of the message queue.
  * @msg_queue_associate:
  *	Check permission when a message queue is requested through the
- *	msgget system call.  This hook is only called when returning the
+ *	msgget system call. This hook is only called when returning the
  *	message queue identifier for an existing message queue, not when a
  *	new message queue is created.
- *	@msq contains the message queue to act upon.
+ *	@perm contains the IPC permissions of the message queue.
  *	@msqflg contains the operation control flags.
  *	Return 0 if permission is granted.
  * @msg_queue_msgctl:
  *	Check permission when a message control operation specified by @cmd
- *	is to be performed on the message queue @msq.
- *	The @msq may be NULL, e.g. for IPC_INFO or MSG_INFO.
- *	@msq contains the message queue to act upon.  May be NULL.
+ *	is to be performed on the message queue with permissions @perm.
+ *	The @perm may be NULL, e.g. for IPC_INFO or MSG_INFO.
+ *	@perm contains the IPC permissions of the msg queue. May be NULL.
  *	@cmd contains the operation to be performed.
  *	Return 0 if permission is granted.
  * @msg_queue_msgsnd:
  *	Check permission before a message, @msg, is enqueued on the message
- *	queue, @msq.
- *	@msq contains the message queue to send message to.
+ *	queue with permissions @perm.
+ *	@perm contains the IPC permissions of the message queue.
  *	@msg contains the message to be enqueued.
  *	@msqflg contains operational flags.
  *	Return 0 if permission is granted.
  * @msg_queue_msgrcv:
  *	Check permission before a message, @msg, is removed from the message
- *	queue, @msq.  The @target task structure contains a pointer to the
+ *	queue. The @target task structure contains a pointer to the
  *	process that will be receiving the message (not equal to the current
  *	process when inline receives are being performed).
- *	@msq contains the message queue to retrieve message from.
+ *	@perm contains the IPC permissions of the message queue.
  *	@msg contains the message destination.
  *	@target contains the task structure for recipient process.
  *	@type contains the type of message requested.
@@ -1615,13 +1615,13 @@ union security_list_options {
 	int (*msg_msg_alloc_security)(struct msg_msg *msg);
 	void (*msg_msg_free_security)(struct msg_msg *msg);
 
-	int (*msg_queue_alloc_security)(struct kern_ipc_perm *msq);
-	void (*msg_queue_free_security)(struct kern_ipc_perm *msq);
-	int (*msg_queue_associate)(struct kern_ipc_perm *msq, int msqflg);
-	int (*msg_queue_msgctl)(struct kern_ipc_perm *msq, int cmd);
-	int (*msg_queue_msgsnd)(struct kern_ipc_perm *msq, struct msg_msg *msg,
+	int (*msg_queue_alloc_security)(struct kern_ipc_perm *perm);
+	void (*msg_queue_free_security)(struct kern_ipc_perm *perm);
+	int (*msg_queue_associate)(struct kern_ipc_perm *perm, int msqflg);
+	int (*msg_queue_msgctl)(struct kern_ipc_perm *perm, int cmd);
+	int (*msg_queue_msgsnd)(struct kern_ipc_perm *perm, struct msg_msg *msg,
 				int msqflg);
-	int (*msg_queue_msgrcv)(struct kern_ipc_perm *msq, struct msg_msg *msg,
+	int (*msg_queue_msgrcv)(struct kern_ipc_perm *perm, struct msg_msg *msg,
 				struct task_struct *target, long type,
 				int mode);
 
-- 
2.17.2

[PATCH 09/10] security: fix documentation for the sem_* hooks

[Denis Efremov]

The sem_* hooks were changed in the commit
"sem/security: Pass kern_ipc_perm not sem_array into the
sem security hooks" (aefad9593ec5). The type of the argument
sma was changed from sem_array to kern_ipc_perm. This patch
updates the documentation for the hooks accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f61f39c73208..4bfb6532cbb3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1172,34 +1172,34 @@
  * Security hooks for System V Semaphores
  *
  * @sem_alloc_security:
- *	Allocate and attach a security structure to the sma->sem_perm.security
- *	field.  The security field is initialized to NULL when the structure is
+ *	Allocate and attach a security structure to the perm->security
+ *	field. The security field is initialized to NULL when the structure is
  *	first created.
- *	@sma contains the semaphore structure
+ *	@perm contains the IPC permissions of the semaphore.
  *	Return 0 if operation was successful and permission is granted.
  * @sem_free_security:
  *	deallocate security struct for this semaphore
- *	@sma contains the semaphore structure.
+ *	@perm contains the IPC permissions of the semaphore.
  * @sem_associate:
  *	Check permission when a semaphore is requested through the semget
- *	system call.  This hook is only called when returning the semaphore
+ *	system call. This hook is only called when returning the semaphore
  *	identifier for an existing semaphore, not when a new one must be
  *	created.
- *	@sma contains the semaphore structure.
+ *	@perm contains the IPC permissions of the semaphore.
  *	@semflg contains the operation control flags.
  *	Return 0 if permission is granted.
  * @sem_semctl:
  *	Check permission when a semaphore operation specified by @cmd is to be
- *	performed on the semaphore @sma.  The @sma may be NULL, e.g. for
+ *	performed on the semaphore. The @perm may be NULL, e.g. for
  *	IPC_INFO or SEM_INFO.
- *	@sma contains the semaphore structure.  May be NULL.
+ *	@perm contains the IPC permissions of the semaphore. May be NULL.
  *	@cmd contains the operation to be performed.
  *	Return 0 if permission is granted.
  * @sem_semop:
  *	Check permissions before performing operations on members of the
- *	semaphore set @sma.  If the @alter flag is nonzero, the semaphore set
+ *	semaphore set. If the @alter flag is nonzero, the semaphore set
  *	may be modified.
- *	@sma contains the semaphore structure.
+ *	@perm contains the IPC permissions of the semaphore.
  *	@sops contains the operations to perform.
  *	@nsops contains the number of operations to perform.
  *	@alter contains the flag indicating whether changes are to be made.
@@ -1632,11 +1632,11 @@ union security_list_options {
 	int (*shm_shmat)(struct kern_ipc_perm *shp, char __user *shmaddr,
 				int shmflg);
 
-	int (*sem_alloc_security)(struct kern_ipc_perm *sma);
-	void (*sem_free_security)(struct kern_ipc_perm *sma);
-	int (*sem_associate)(struct kern_ipc_perm *sma, int semflg);
-	int (*sem_semctl)(struct kern_ipc_perm *sma, int cmd);
-	int (*sem_semop)(struct kern_ipc_perm *sma, struct sembuf *sops,
+	int (*sem_alloc_security)(struct kern_ipc_perm *perm);
+	void (*sem_free_security)(struct kern_ipc_perm *perm);
+	int (*sem_associate)(struct kern_ipc_perm *perm, int semflg);
+	int (*sem_semctl)(struct kern_ipc_perm *perm, int cmd);
+	int (*sem_semop)(struct kern_ipc_perm *perm, struct sembuf *sops,
 				unsigned nsops, int alter);
 
 	int (*netlink_send)(struct sock *sk, struct sk_buff *skb);
-- 
2.17.2

[PATCH 10/10] security: fix documentation for the shm_* hooks

[Denis Efremov]

The shm_* hooks were changed in the commit
"shm/security: Pass kern_ipc_perm not shmid_kernel into the
shm security hooks" (7191adff2a55). The type of the argument
shp was changed from shmid_kernel to kern_ipc_perm. This patch
updates the documentation for the hooks accordingly.

Signed-off-by: Denis Efremov <efremov@ispras.ru>
---
 include/linux/lsm_hooks.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4bfb6532cbb3..8382dd1bed59 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1137,34 +1137,34 @@
  * Security hooks for System V Shared Memory Segments
  *
  * @shm_alloc_security:
- *	Allocate and attach a security structure to the shp->shm_perm.security
- *	field.  The security field is initialized to NULL when the structure is
+ *	Allocate and attach a security structure to the perm->security
+ *	field. The security field is initialized to NULL when the structure is
  *	first created.
- *	@shp contains the shared memory structure to be modified.
+ *	@perm contains the IPC permissions of the shared memory structure.
  *	Return 0 if operation was successful and permission is granted.
  * @shm_free_security:
  *	Deallocate the security struct for this memory segment.
- *	@shp contains the shared memory structure to be modified.
+ *	@perm contains the IPC permissions of the shared memory structure.
  * @shm_associate:
  *	Check permission when a shared memory region is requested through the
- *	shmget system call.  This hook is only called when returning the shared
+ *	shmget system call. This hook is only called when returning the shared
  *	memory region identifier for an existing region, not when a new shared
  *	memory region is created.
- *	@shp contains the shared memory structure to be modified.
+ *	@perm contains the IPC permissions of the shared memory structure.
  *	@shmflg contains the operation control flags.
  *	Return 0 if permission is granted.
  * @shm_shmctl:
  *	Check permission when a shared memory control operation specified by
- *	@cmd is to be performed on the shared memory region @shp.
- *	The @shp may be NULL, e.g. for IPC_INFO or SHM_INFO.
- *	@shp contains shared memory structure to be modified.
+ *	@cmd is to be performed on the shared memory region with permissions @perm.
+ *	The @perm may be NULL, e.g. for IPC_INFO or SHM_INFO.
+ *	@perm contains the IPC permissions of the shared memory structure.
  *	@cmd contains the operation to be performed.
  *	Return 0 if permission is granted.
  * @shm_shmat:
  *	Check permissions prior to allowing the shmat system call to attach the
- *	shared memory segment @shp to the data segment of the calling process.
- *	The attaching address is specified by @shmaddr.
- *	@shp contains the shared memory structure to be modified.
+ *	shared memory segment with permissions @perm to the data segment of the
+ *	calling process. The attaching address is specified by @shmaddr.
+ *	@perm contains the IPC permissions of the shared memory structure.
  *	@shmaddr contains the address to attach memory region to.
  *	@shmflg contains the operational flags.
  *	Return 0 if permission is granted.
@@ -1625,11 +1625,11 @@ union security_list_options {
 				struct task_struct *target, long type,
 				int mode);
 
-	int (*shm_alloc_security)(struct kern_ipc_perm *shp);
-	void (*shm_free_security)(struct kern_ipc_perm *shp);
-	int (*shm_associate)(struct kern_ipc_perm *shp, int shmflg);
-	int (*shm_shmctl)(struct kern_ipc_perm *shp, int cmd);
-	int (*shm_shmat)(struct kern_ipc_perm *shp, char __user *shmaddr,
+	int (*shm_alloc_security)(struct kern_ipc_perm *perm);
+	void (*shm_free_security)(struct kern_ipc_perm *perm);
+	int (*shm_associate)(struct kern_ipc_perm *perm, int shmflg);
+	int (*shm_shmctl)(struct kern_ipc_perm *perm, int cmd);
+	int (*shm_shmat)(struct kern_ipc_perm *perm, char __user *shmaddr,
 				int shmflg);
 
 	int (*sem_alloc_security)(struct kern_ipc_perm *perm);
-- 
2.17.2

Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[Al Viro]

On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote:
> The path_chmod hook was changed in the commit
> "switch security_path_chmod() to struct path *" (cdcf116d44e7).
> The argument @mnt was removed from the hook, @dentry was changed
> to @path. This patch updates the documentation accordingly.
> 
> Signed-off-by: Denis Efremov <efremov@ispras.ru>
> ---
>  include/linux/lsm_hooks.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index cb93972257be..5d6428d0027b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -304,8 +304,7 @@
>   *	Return 0 if permission is granted.
>   * @path_chmod:
>   *	Check for permission to change DAC's permission of a file or directory.
> - *	@dentry contains the dentry structure.
> - *	@mnt contains the vfsmnt structure.
> + *	@path contains the path structure.

May I politely inquire about the value of these comments?  How much information
is provided by refering to an argument as "the dentry structure" or "the path
structure", especially when there's nothing immediately above that would introduce
either.  "Type of 'dentry' argument is somehow related to struct dentry,
try and guess what the value might be - we don't care, we just need every
argument commented"?

Who needs that crap in the first place?

RE: [PATCH 06/10] security: fix documentation for the path_chmod hook

[Edwin Zimmerman]

On Thursday, February 07, 2019 8:50 AM Al Viro wrote:
> On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote:
> > The path_chmod hook was changed in the commit
> > "switch security_path_chmod() to struct path *" (cdcf116d44e7).
> > The argument @mnt was removed from the hook, @dentry was changed
> > to @path. This patch updates the documentation accordingly.
> >
> > Signed-off-by: Denis Efremov <efremov@ispras.ru>
> > ---
> >  include/linux/lsm_hooks.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index cb93972257be..5d6428d0027b 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -304,8 +304,7 @@
> >   *	Return 0 if permission is granted.
> >   * @path_chmod:
> >   *	Check for permission to change DAC's permission of a file or directory.
> > - *	@dentry contains the dentry structure.
> > - *	@mnt contains the vfsmnt structure.
> > + *	@path contains the path structure.
> 
> May I politely inquire about the value of these comments?  How much information
> is provided by refering to an argument as "the dentry structure" or "the path
> structure", especially when there's nothing immediately above that would introduce
> either.  "Type of 'dentry' argument is somehow related to struct dentry,
> try and guess what the value might be - we don't care, we just need every
> argument commented"?
> 
> Who needs that crap in the first place?

The comments fill a valuable place to folks like me who are new to the linux security modules.
In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the
comments in lsm_hooks.h have helped me out more than once.  Perhaps the comments could
be inproved by changing them to something like this:
"@[arg] contains the [type] structure, defined in linux/[?].h"

Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[Stephen Smalley]

On 2/7/19 9:09 AM, Edwin Zimmerman wrote:
> On Thursday, February 07, 2019 8:50 AM Al Viro wrote:
>> On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote:
>>> The path_chmod hook was changed in the commit
>>> "switch security_path_chmod() to struct path *" (cdcf116d44e7).
>>> The argument @mnt was removed from the hook, @dentry was changed
>>> to @path. This patch updates the documentation accordingly.
>>>
>>> Signed-off-by: Denis Efremov <efremov@ispras.ru>
>>> ---
>>>   include/linux/lsm_hooks.h | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index cb93972257be..5d6428d0027b 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -304,8 +304,7 @@
>>>    *	Return 0 if permission is granted.
>>>    * @path_chmod:
>>>    *	Check for permission to change DAC's permission of a file or directory.
>>> - *	@dentry contains the dentry structure.
>>> - *	@mnt contains the vfsmnt structure.
>>> + *	@path contains the path structure.
>>
>> May I politely inquire about the value of these comments?  How much information
>> is provided by refering to an argument as "the dentry structure" or "the path
>> structure", especially when there's nothing immediately above that would introduce
>> either.  "Type of 'dentry' argument is somehow related to struct dentry,
>> try and guess what the value might be - we don't care, we just need every
>> argument commented"?
>>
>> Who needs that crap in the first place?
> 
> The comments fill a valuable place to folks like me who are new to the linux security modules.
> In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the
> comments in lsm_hooks.h have helped me out more than once.  Perhaps the comments could
> be inproved by changing them to something like this:
> "@[arg] contains the [type] structure, defined in linux/[?].h"

I don't think so.  The point is not what type of structure but what 
object is being passed and why is it relevant to the hook, e.g.

+ @path contains the path structure for the file whose permissions are 
being modified

or similar.

Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[Al Viro]

On Thu, Feb 07, 2019 at 09:09:49AM -0500, Edwin Zimmerman wrote:
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index cb93972257be..5d6428d0027b 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -304,8 +304,7 @@
> > >   *	Return 0 if permission is granted.
> > >   * @path_chmod:
> > >   *	Check for permission to change DAC's permission of a file or directory.
> > > - *	@dentry contains the dentry structure.
> > > - *	@mnt contains the vfsmnt structure.
> > > + *	@path contains the path structure.
> > 
> > May I politely inquire about the value of these comments?  How much information
> > is provided by refering to an argument as "the dentry structure" or "the path
> > structure", especially when there's nothing immediately above that would introduce
> > either.  "Type of 'dentry' argument is somehow related to struct dentry,
> > try and guess what the value might be - we don't care, we just need every
> > argument commented"?
> > 
> > Who needs that crap in the first place?
> 
> The comments fill a valuable place to folks like me who are new to the linux security modules.
> In my spare time, I'm writing a new LSM specifically geared for parental controls uses, and the
> comments in lsm_hooks.h have helped me out more than once.  Perhaps the comments could
> be inproved by changing them to something like this:
> "@[arg] contains the [type] structure, defined in linux/[?].h"

Um...   The _type_ of argument is visible in declaration already;
it doesn't say a damn thing about the value of that argument.

In this particular case, dentry/mnt pair (whichever way it gets
passed; struct path is exactly such a pair) is actually used to
specify the location of file or directory in question, but
try to guess that by description given in this "documentation"...

As for "defined in"... that's what grep/ctags/etc. are for.
Again, the useful information about an argument is _what_ gets
passed in it, not just which type it is...

Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[Stephen Smalley]

On 2/7/19 9:32 AM, Stephen Smalley wrote:
> On 2/7/19 9:09 AM, Edwin Zimmerman wrote:
>> On Thursday, February 07, 2019 8:50 AM Al Viro wrote:
>>> On Thu, Feb 07, 2019 at 03:44:54PM +0300, Denis Efremov wrote:
>>>> The path_chmod hook was changed in the commit
>>>> "switch security_path_chmod() to struct path *" (cdcf116d44e7).
>>>> The argument @mnt was removed from the hook, @dentry was changed
>>>> to @path. This patch updates the documentation accordingly.
>>>>
>>>> Signed-off-by: Denis Efremov <efremov@ispras.ru>
>>>> ---
>>>>   include/linux/lsm_hooks.h | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>>> index cb93972257be..5d6428d0027b 100644
>>>> --- a/include/linux/lsm_hooks.h
>>>> +++ b/include/linux/lsm_hooks.h
>>>> @@ -304,8 +304,7 @@
>>>>    *    Return 0 if permission is granted.
>>>>    * @path_chmod:
>>>>    *    Check for permission to change DAC's permission of a file or 
>>>> directory.
>>>> - *    @dentry contains the dentry structure.
>>>> - *    @mnt contains the vfsmnt structure.
>>>> + *    @path contains the path structure.
>>>
>>> May I politely inquire about the value of these comments?  How much 
>>> information
>>> is provided by refering to an argument as "the dentry structure" or 
>>> "the path
>>> structure", especially when there's nothing immediately above that 
>>> would introduce
>>> either.  "Type of 'dentry' argument is somehow related to struct dentry,
>>> try and guess what the value might be - we don't care, we just need 
>>> every
>>> argument commented"?
>>>
>>> Who needs that crap in the first place?
>>
>> The comments fill a valuable place to folks like me who are new to the 
>> linux security modules.
>> In my spare time, I'm writing a new LSM specifically geared for 
>> parental controls uses, and the
>> comments in lsm_hooks.h have helped me out more than once.  Perhaps 
>> the comments could
>> be inproved by changing them to something like this:
>> "@[arg] contains the [type] structure, defined in linux/[?].h"
> 
> I don't think so.  The point is not what type of structure but what 
> object is being passed and why is it relevant to the hook, e.g.
> 
> + @path contains the path structure for the file whose permissions are 
> being modified
> 
> or similar.

It would probably be better to amend the description too to refer to the 
argument in context, e.g.

* @path_chmod:
*     Check for permission to change the mode of the file referenced by 
@path.
*     @path the file whose mode would be modified

or similar.

I'd suggest looking to kerneldoc comments in fs/*.c or elsewhere as 
better examples.

Re: [PATCH 00/10] LSM documentation update

[Kees Cook]

On Thu, Feb 7, 2019 at 4:45 AM Denis Efremov <efremov@ispras.ru> wrote:
>
> Recent "New LSM Hooks" discussion has led me to the
> thought that it might be a good idea to slightly
> update the current documentation. The patchset adds
> nothing new to the documentation, only fixes the old
> description of hooks to reflect their current state.

Do these fixes silence any warnings during the documentation build?
(i.e. how did you verify the results beyond eyeballing the changes)

-Kees

>
> Denis Efremov (10):
>   security: fix documentation for the sb_copy_data hook
>   security: fix documentation for the syslog hook
>   security: fix documentation for the socket_post_create hook
>   security: fix documentation for the task_setscheduler hook
>   security: fix documentation for the socket_getpeersec_dgram hook
>   security: fix documentation for the path_chmod hook
>   security: fix documentation for the audit_* hooks
>   security: fix documentation for the msg_queue_* hooks
>   security: fix documentation for the sem_* hooks
>   security: fix documentation for the shm_* hooks
>
>  include/linux/lsm_hooks.h | 127 ++++++++++++++++++--------------------
>  1 file changed, 61 insertions(+), 66 deletions(-)
>
> --
> 2.17.2
>


-- 
Kees Cook

Re: [PATCH 00/10] LSM documentation update

[efremov]

Kees Cook писал 2019-02-11 22:28:
> On Thu, Feb 7, 2019 at 4:45 AM Denis Efremov <efremov@ispras.ru> wrote:
>> 
>> Recent "New LSM Hooks" discussion has led me to the
>> thought that it might be a good idea to slightly
>> update the current documentation. The patchset adds
>> nothing new to the documentation, only fixes the old
>> description of hooks to reflect their current state.

> 
> Do these fixes silence any warnings during the documentation build?
> (i.e. how did you verify the results beyond eyeballing the changes)
> 
> -Kees

This LSM documentation is not used during the documentation build.
At least I can't find it in the resulting build directory and at
the online documentation on the kernel. Most of the fixes are pretty
obvious and can be checked by comparing an lsm hook declaration and
its description in the LSM comment from lsm_hooks.h I tried to be
exhaustive in the commits description and in every case to reference
the original commit where the interface was changed without
documentation update.

> 
>> 
>> Denis Efremov (10):
>>   security: fix documentation for the sb_copy_data hook
>>   security: fix documentation for the syslog hook
>>   security: fix documentation for the socket_post_create hook
>>   security: fix documentation for the task_setscheduler hook
>>   security: fix documentation for the socket_getpeersec_dgram hook
>>   security: fix documentation for the path_chmod hook
>>   security: fix documentation for the audit_* hooks
>>   security: fix documentation for the msg_queue_* hooks
>>   security: fix documentation for the sem_* hooks
>>   security: fix documentation for the shm_* hooks
>> 
>>  include/linux/lsm_hooks.h | 127 
>> ++++++++++++++++++--------------------
>>  1 file changed, 61 insertions(+), 66 deletions(-)
>> 
>> --
>> 2.17.2
>> 

Re: [PATCH 06/10] security: fix documentation for the path_chmod hook

[efremov]

Al Viro писал 2019-02-07 17:46:
> On Thu, Feb 07, 2019 at 09:09:49AM -0500, Edwin Zimmerman wrote:
>> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> > > index cb93972257be..5d6428d0027b 100644
>> > > --- a/include/linux/lsm_hooks.h
>> > > +++ b/include/linux/lsm_hooks.h
>> > > @@ -304,8 +304,7 @@
>> > >   *	Return 0 if permission is granted.
>> > >   * @path_chmod:
>> > >   *	Check for permission to change DAC's permission of a file or directory.
>> > > - *	@dentry contains the dentry structure.
>> > > - *	@mnt contains the vfsmnt structure.
>> > > + *	@path contains the path structure.
>> >
>> > May I politely inquire about the value of these comments?  How much information
>> > is provided by refering to an argument as "the dentry structure" or "the path
>> > structure", especially when there's nothing immediately above that would introduce
>> > either.  "Type of 'dentry' argument is somehow related to struct dentry,
>> > try and guess what the value might be - we don't care, we just need every
>> > argument commented"?
>> >
>> > Who needs that crap in the first place?
>> 
>> The comments fill a valuable place to folks like me who are new to the 
>> linux security modules.
>> In my spare time, I'm writing a new LSM specifically geared for 
>> parental controls uses, and the
>> comments in lsm_hooks.h have helped me out more than once.  Perhaps 
>> the comments could
>> be inproved by changing them to something like this:
>> "@[arg] contains the [type] structure, defined in linux/[?].h"
> 
> Um...   The _type_ of argument is visible in declaration already;
> it doesn't say a damn thing about the value of that argument.
> 
> In this particular case, dentry/mnt pair (whichever way it gets
> passed; struct path is exactly such a pair) is actually used to
> specify the location of file or directory in question, but
> try to guess that by description given in this "documentation"...
> 
> As for "defined in"... that's what grep/ctags/etc. are for.
> Again, the useful information about an argument is _what_ gets
> passed in it, not just which type it is...

While I completely agree about the doubtful value of such comments,
the whole documentation is written in style like that. Unfortunately,
I don't have the knowledge to rewrite the documentation in every case
even for functions from this patchset. And I will be surprised if a
single person could do it for the whole LSM set.
The patchset only minimally updates the documentation to lift it up
to the current LSM declarations. And maybe to show once again that
despite we've got the documentation on a hook it maybe not actual
for 12 years since the hook was changed in 2006. But of course, it
doesn't mean that documentation is not needed.
This can give the maintainers additional arguments for stricter checks
before accepting new hooks and changing the existing ones.
I will try to improve the description in the second version of the
patchset.

Re: [PATCH 00/10] LSM documentation update

[Kees Cook]

On Sun, Feb 17, 2019 at 10:04 AM efremov <efremov@ispras.ru> wrote:
>
> Kees Cook писал 2019-02-11 22:28:
> > On Thu, Feb 7, 2019 at 4:45 AM Denis Efremov <efremov@ispras.ru> wrote:
> >>
> >> Recent "New LSM Hooks" discussion has led me to the
> >> thought that it might be a good idea to slightly
> >> update the current documentation. The patchset adds
> >> nothing new to the documentation, only fixes the old
> >> description of hooks to reflect their current state.
>
> >
> > Do these fixes silence any warnings during the documentation build?
> > (i.e. how did you verify the results beyond eyeballing the changes)
> >
> > -Kees
>
> This LSM documentation is not used during the documentation build.
> At least I can't find it in the resulting build directory and at
> the online documentation on the kernel. Most of the fixes are pretty
> obvious and can be checked by comparing an lsm hook declaration and
> its description in the LSM comment from lsm_hooks.h I tried to be
> exhaustive in the commits description and in every case to reference
> the original commit where the interface was changed without
> documentation update.

I'll send an official patch that'll hook this up. I've been meaning to
do it for a while, but there were several things that needed cleaning
up (and you've snagged most of them). So with this patch, you'll be
able to check the output for "make htmldocs" for lsm_hooks.h errors:

diff --git a/Documentation/security/LSM.rst b/Documentation/security/LSM.rst
index 8b9ee597e9d0..31d92bc5fdd2 100644
--- a/Documentation/security/LSM.rst
+++ b/Documentation/security/LSM.rst
@@ -11,4 +11,7 @@ that end users and distros can make a more informed
decision about which
 LSMs suit their requirements.

 For extensive documentation on the available LSM hook interfaces, please
-see ``include/linux/lsm_hooks.h``.
+see ``include/linux/lsm_hooks.h`` and associated structures:
+
+.. kernel-doc:: include/linux/lsm_hooks.h
+   :internal:


-- 
Kees Cook