[PATCH 1/2] misc: nsm: require CAP_SYS_ADMIN for raw ioctl

[PATCH 1/2] misc: nsm: require CAP_SYS_ADMIN for raw ioctl

[Vu Nguyen Anh Khoa]

NSM_IOCTL_RAW lets userspace submit raw NSM messages. The UAPI
documents this ioctl as available only with CAP_SYS_ADMIN, but /dev/nsm
is registered with mode 0666 and nsm_dev_ioctl() does not enforce that
restriction.

Reject unprivileged raw ioctl requests before accepting user-controlled
NSM messages.

Signed-off-by: Vu Nguyen Anh Khoa <khoavna.tin.2225@gmail.com>
---
 drivers/misc/nsm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/misc/nsm.c b/drivers/misc/nsm.c
index ef7b32742..52790df7d 100644
--- a/drivers/misc/nsm.c
+++ b/drivers/misc/nsm.c
@@ -9,6 +9,7 @@
  * space can use to issue these commands.
  */
 
+#include <linux/capability.h>
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -361,6 +362,9 @@ static long nsm_dev_ioctl(struct file *file, unsigned int cmd,
 	if (cmd != NSM_IOCTL_RAW)
 		return -EINVAL;
 
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	if (_IOC_SIZE(cmd) != sizeof(raw))
 		return -EINVAL;
 
-- 
2.43.0

[PATCH 2/2] misc: nsm: do not unlock mutex before locking it

[Vu Nguyen Anh Khoa]

nsm_dev_ioctl() jumps to the common out label when the initial
copy_from_user() fails. That failure path runs before
mutex_lock(&nsm->lock), but the out label unconditionally calls
mutex_unlock(&nsm->lock).

Return -EFAULT directly for the pre-lock copy_from_user() failure so
only paths that acquired the mutex release it.

Signed-off-by: Vu Nguyen Anh Khoa <khoavna.tin.2225@gmail.com>
---
 drivers/misc/nsm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/misc/nsm.c b/drivers/misc/nsm.c
index 52790df7d..8153edff3 100644
--- a/drivers/misc/nsm.c
+++ b/drivers/misc/nsm.c
@@ -369,9 +369,8 @@ static long nsm_dev_ioctl(struct file *file, unsigned int cmd,
 		return -EINVAL;
 
 	/* Copy user argument struct to kernel argument struct */
-	r = -EFAULT;
 	if (copy_from_user(&raw, argp, _IOC_SIZE(cmd)))
-		goto out;
+		return -EFAULT;
 
 	mutex_lock(&nsm->lock);
 
-- 
2.43.0

Re: [PATCH 1/2] misc: nsm: require CAP_SYS_ADMIN for raw ioctl

[Graf (AWS), Alexander]

On 21.06.26 10:57, Vu Nguyen Anh Khoa wrote:
> NSM_IOCTL_RAW lets userspace submit raw NSM messages. The UAPI
> documents this ioctl as available only with CAP_SYS_ADMIN, but /dev/nsm
> is registered with mode 0666 and nsm_dev_ioctl() does not enforce that
> restriction.
>
> Reject unprivileged raw ioctl requests before accepting user-controlled
> NSM messages.
>
> Signed-off-by: Vu Nguyen Anh Khoa <khoavna.tin.2225@gmail.com>


This must have fallen through the cracks when I juggled with the 
different versions during initial submission. Nice catch!

Reviewed-by: Alexander Graf <graf@amazon.com>


Alex



Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christof Hellmis, Andreas Stieger
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Re: [PATCH 2/2] misc: nsm: do not unlock mutex before locking it

[Graf (AWS), Alexander]

On 21.06.26 10:57, Vu Nguyen Anh Khoa wrote:
> nsm_dev_ioctl() jumps to the common out label when the initial
> copy_from_user() fails. That failure path runs before
> mutex_lock(&nsm->lock), but the out label unconditionally calls
> mutex_unlock(&nsm->lock).
>
> Return -EFAULT directly for the pre-lock copy_from_user() failure so
> only paths that acquired the mutex release it.
>
> Signed-off-by: Vu Nguyen Anh Khoa <khoavna.tin.2225@gmail.com>


Reviewed-by: Alexander Graf <graf@amazon.com>


Alex




Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christof Hellmis, Andreas Stieger
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

Re: [PATCH 1/2] misc: nsm: require CAP_SYS_ADMIN for raw ioctl

[Graf (AWS), Alexander]

On 21.06.26 11:33, Alexander Graf wrote:
>
> On 21.06.26 10:57, Vu Nguyen Anh Khoa wrote:
>> NSM_IOCTL_RAW lets userspace submit raw NSM messages. The UAPI
>> documents this ioctl as available only with CAP_SYS_ADMIN, but /dev/nsm
>> is registered with mode 0666 and nsm_dev_ioctl() does not enforce that
>> restriction.
>>
>> Reject unprivileged raw ioctl requests before accepting user-controlled
>> NSM messages.
>>
>> Signed-off-by: Vu Nguyen Anh Khoa <khoavna.tin.2225@gmail.com>
>
>
> This must have fallen through the cracks when I juggled with the 
> different versions during initial submission. Nice catch!
>
> Reviewed-by: Alexander Graf <graf@amazon.com>
>

Actually, thinking a bit harder about it, I think this may break legit 
use cases if you for example want to pass NSM access to a deprivileged 
container namespace.

Greg, please drop this patch. It will break existing user space.

Vu, can you please instead patch the uapi header so it doesn't say "only 
CAP_ADMIN"? That was a remnant of when the device had individual ioctls 
for each command you could invoke, which turned out too ugly to pull 
into the kernel. When reverting back to the "raw only" interface we have 
today, I forgot to remove the CAP_ADMIN note.


Alex



Amazon Web Services Development Center Germany GmbH
Tamara-Danz-Str. 13
10243 Berlin
Geschaeftsfuehrung: Christof Hellmis, Andreas Stieger
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597