[PATCH v1 0/2] lsm: Preserve full ioctl commands in audit records

[PATCH v1 0/2] lsm: Preserve full ioctl commands in audit records

[Mickaël Salaün]

Hi,

Ioctl commands are unsigned int values, with conventionally encoded size
and direction fields above bit 15.  However, struct lsm_ioctlop_audit
stores only a u16 and the common audit formatter uses %hx.  SELinux also
narrows the command before constructing this audit data.  As a result,
LSM audit records can conflate distinct ioctl requests; for example,
Landlock currently records 0xc00ffeee as 0xfeee.

The narrow audit field dates to its 2015 introduction [1] alongside
SELinux ioctl extended permissions.  Using the low type and number bytes
for those policy checks was deliberate, but the mailing-list review
contains no separate decision to truncate the audit value.  Audit
discussion instead identified ioctlcmd with the ioctl request in syscall
argument a1, and the audit field dictionary defines it as the request
argument to ioctl.

This also exposes an audit2allow -x assumption: it copies ioctlcmd
directly into a 16-bit SELinux extended-permission rule.  It needs to
derive that rule from the low 16 bits of the full audited request;
otherwise checkpolicy rejects generated values above 0xffff.

Patch 1 preserves the full command in shared audit data without changing
SELinux's low-16-bit policy checks.  Patch 2 updates the existing
Landlock test to exercise upper command bits.

[1] https://lore.kernel.org/r/1428616171-14767-1-git-send-email-jeffv@google.com

Mickaël Salaün (2):
  lsm: Preserve full ioctl commands in audit records
  selftests/landlock: Check full ioctl commands in audit records

 include/linux/lsm_audit.h                  |  2 +-
 security/lsm_audit.c                       |  2 +-
 security/selinux/hooks.c                   |  6 +++---
 tools/testing/selftests/landlock/fs_test.c | 10 +++++++---
 4 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.55.0

[PATCH v1 1/2] lsm: Preserve full ioctl commands in audit records

[Mickaël Salaün]

Ioctl commands are unsigned int values, with their encoded direction and
size in the upper 16 bits.  However, struct lsm_ioctlop_audit keeps only
a u16, and the common audit formatter uses %hx.  This logs 0xc00ffeee as
0xfeee and makes distinct commands with matching low 16 bits
indistinguishable.

Change cmd to unsigned int and use %x.  Pass the full command through
ioctl_has_perm() as well.  Its driver and xperm values remain u8 values
derived from the same low 16 bits, so SELinux enforcement does not
change.  Commands that fit in 16 bits keep the same audit text.

Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: stable@vger.kernel.org
Fixes: 671a2781ff01 ("security: add ioctl specific auditing to lsm_audit")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 include/linux/lsm_audit.h | 2 +-
 security/lsm_audit.c      | 2 +-
 security/selinux/hooks.c  | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 584db296e43b..fc68ca94f05a 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -44,7 +44,7 @@ struct lsm_network_audit {
 
 struct lsm_ioctlop_audit {
 	struct path path;
-	u16 cmd;
+	unsigned int cmd;
 };
 
 struct lsm_ibpkey_audit {
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 737f5a263a8f..a7891a75d0aa 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -231,7 +231,7 @@ void audit_log_lsm_data(struct audit_buffer *ab,
 			audit_log_format(ab, " ino=%llu", inode->i_ino);
 		}
 
-		audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
+		audit_log_format(ab, " ioctlcmd=0x%x", a->u.op->cmd);
 		break;
 	}
 	case LSM_AUDIT_DATA_DENTRY: {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 035aaf113d1d..e3eaa3141d91 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3859,7 +3859,7 @@ static int selinux_backing_file_alloc(struct file *backing_file,
  * operation to an inode.
  */
 static int ioctl_has_perm(const struct cred *cred, struct file *file,
-		u32 requested, u16 cmd)
+		u32 requested, unsigned int cmd)
 {
 	struct common_audit_data ad;
 	struct file_security_struct *fsec = selinux_file(file);
@@ -3930,14 +3930,14 @@ static int selinux_file_ioctl(struct file *file, unsigned int cmd,
 	case FIOCLEX:
 	case FIONCLEX:
 		if (!selinux_policycap_ioctl_skip_cloexec())
-			error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
+			error = ioctl_has_perm(cred, file, FILE__IOCTL, cmd);
 		break;
 
 	/* default case assumes that the command will go
 	 * to the file's ioctl() function.
 	 */
 	default:
-		error = ioctl_has_perm(cred, file, FILE__IOCTL, (u16) cmd);
+		error = ioctl_has_perm(cred, file, FILE__IOCTL, cmd);
 	}
 	return error;
 }
-- 
2.55.0

[PATCH v1 2/2] selftests/landlock: Check full ioctl commands in audit records

[Mickaël Salaün]

The ioctl audit test uses FIONREAD, whose command value fits in 16 bits.
It therefore cannot detect truncation of encoded direction and size
bits.

Use an architecture-independent unknown command with set upper bits and
require its complete value in the audit record.  Landlock rejects the
command before the device handles it, and the existing ioctl enforcement
test already exercises the same request.  The new expectation fails with
ioctlcmd=0xfeee on an unfixed kernel and passes with ioctlcmd=0xc00ffeee
after the shared audit field is widened.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 tools/testing/selftests/landlock/fs_test.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 18dbdb99aeba..1abdcc9ee784 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4146,6 +4146,9 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
 	ASSERT_EQ(0, close(fd));
 }
 
+/* Arbitrary command with nonzero bits in both 16-bit halves. */
+static const unsigned int unknown_ioctl_cmd = 0xc00ffeee;
+
 /*
  * ioctl_error - generically call the given ioctl with a pointer to a
  * sufficiently large zeroed-out memory region.
@@ -4249,7 +4252,7 @@ TEST_F_FORK(layout1, blanket_permitted_ioctls)
 	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, FS_IOC_ZERO_RANGE));
 
 	/* Default case is also blocked. */
-	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, 0xc00ffeee));
+	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, unknown_ioctl_cmd));
 
 	ASSERT_EQ(0, close(fd));
 }
@@ -7943,6 +7946,7 @@ TEST_F(audit_layout1, truncate)
 	EXPECT_EQ(1, records.domain);
 }
 
+/* Checks that audit records preserve every ioctl command bit. */
 TEST_F(audit_layout1, ioctl_dev)
 {
 	struct audit_records records;
@@ -7952,10 +7956,10 @@ TEST_F(audit_layout1, ioctl_dev)
 
 	fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
 	ASSERT_LE(0, fd);
-	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, FIONREAD));
+	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, unknown_ioctl_cmd));
 	EXPECT_EQ(0, matches_log_fs_extra(_metadata, self->audit_fd,
 					  "fs\\.ioctl_dev", "/dev/null",
-					  " ioctlcmd=0x541b"));
+					  " ioctlcmd=0xc00ffeee"));
 
 	EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
 	EXPECT_EQ(0, records.access);
-- 
2.55.0

Re: [PATCH v1 2/2] selftests/landlock: Check full ioctl commands in audit records

[Günther Noack]

On Thu, Sep 10, 2026 at 04:48:34PM +0200, Mickaël Salaün wrote:
> The ioctl audit test uses FIONREAD, whose command value fits in 16 bits.
> It therefore cannot detect truncation of encoded direction and size
> bits.
> 
> Use an architecture-independent unknown command with set upper bits and
> require its complete value in the audit record.  Landlock rejects the
> command before the device handles it, and the existing ioctl enforcement
> test already exercises the same request.  The new expectation fails with
> ioctlcmd=0xfeee on an unfixed kernel and passes with ioctlcmd=0xc00ffeee
> after the shared audit field is widened.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  tools/testing/selftests/landlock/fs_test.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 18dbdb99aeba..1abdcc9ee784 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4146,6 +4146,9 @@ TEST_F_FORK(layout1, o_path_ftruncate_and_ioctl)
>  	ASSERT_EQ(0, close(fd));
>  }
>  
> +/* Arbitrary command with nonzero bits in both 16-bit halves. */
> +static const unsigned int unknown_ioctl_cmd = 0xc00ffeee;
> +
>  /*
>   * ioctl_error - generically call the given ioctl with a pointer to a
>   * sufficiently large zeroed-out memory region.
> @@ -4249,7 +4252,7 @@ TEST_F_FORK(layout1, blanket_permitted_ioctls)
>  	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, FS_IOC_ZERO_RANGE));
>  
>  	/* Default case is also blocked. */
> -	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, 0xc00ffeee));
> +	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, unknown_ioctl_cmd));
>  
>  	ASSERT_EQ(0, close(fd));
>  }
> @@ -7943,6 +7946,7 @@ TEST_F(audit_layout1, truncate)
>  	EXPECT_EQ(1, records.domain);
>  }
>  
> +/* Checks that audit records preserve every ioctl command bit. */
>  TEST_F(audit_layout1, ioctl_dev)
>  {
>  	struct audit_records records;
> @@ -7952,10 +7956,10 @@ TEST_F(audit_layout1, ioctl_dev)
>  
>  	fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
>  	ASSERT_LE(0, fd);
> -	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, FIONREAD));
> +	EXPECT_EQ(EACCES, ioctl_error(_metadata, fd, unknown_ioctl_cmd));
>  	EXPECT_EQ(0, matches_log_fs_extra(_metadata, self->audit_fd,
>  					  "fs\\.ioctl_dev", "/dev/null",
> -					  " ioctlcmd=0x541b"));
> +					  " ioctlcmd=0xc00ffeee"));

Minor: I don't see much upside in extracting the shared unknown_ioctl_cmd
variable here; if we used the 0xc00ffeee literally in the
ioctl_error() call here, it would be clearer in this test how the
test's stimulus of calling ioctl() with 0xc00ffeee related to the
expected output, without having to do that side-lookup.

Logic looks good though, good to test it more exhaustively.

>  
>  	EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
>  	EXPECT_EQ(0, records.access);
> -- 
> 2.55.0
> 

Reviewed-by: Günther Noack <gnoack@google.com>