x86, mpx: Ensure unused arguments of prctl() MPX requests are 0

PATCH In review Michael Kerrisk (man-pages) 3 participants 7 messages Jan 8, 2015 – Jan 14, 2015 Memory management

lore

Share & tools

lore

Michael Kerrisk (man-pages)

author

2015-01-08 10:28 UTC #0 lore raw

PATCH 1/1 x86, mpx: Ensure unused arguments of prctl() MPX requests are 0

From: Michael Kerrisk <[email protected]>

commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.

Signed-off-by Michael Kerrisk <[email protected]>

---
 kernel/sys.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sys.c +4 -0

index a8c9f5a..ea9c881 100644
@@ -2210,9 +2210,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		up_write(&me->mm->mmap_sem);
 		break;
 	case PR_MPX_ENABLE_MANAGEMENT:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
 		error = MPX_ENABLE_MANAGEMENT(me);
 		break;
 	case PR_MPX_DISABLE_MANAGEMENT:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
 		error = MPX_DISABLE_MANAGEMENT(me);
 		break;
 	default:

Signature · 11 lines

1.9.3
-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andi Kleen

2015-01-09 17:25 UTC #1 lore raw

Quoted 7 lines · Michael Kerrisk (man-pages)

From: Michael Kerrisk <[email protected]>
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.

This will break the existing gcc run time, which doesn't zero these arguments.

-ANdi

Quoted 26 lines

Signed-off-by Michael Kerrisk <[email protected]>

---
 kernel/sys.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sys.c +4 -0

index a8c9f5a..ea9c881 100644
@@ -2210,9 +2210,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		up_write(&me->mm->mmap_sem);
 		break;
 	case PR_MPX_ENABLE_MANAGEMENT:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
 		error = MPX_ENABLE_MANAGEMENT(me);
 		break;
 	case PR_MPX_DISABLE_MANAGEMENT:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
 		error = MPX_DISABLE_MANAGEMENT(me);
 		break;
 	default:

Signature · 1 line

1.9.3

Signature · 7 lines

[email protected] -- Speaking for myself only

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael Kerrisk (man-pages)

author

2015-01-09 18:26 UTC #2 lore raw

Quoted 4 lines · Andi Kleen

Quoted 2 lines · Michael Kerrisk (man-pages)
From: Michael Kerrisk <[email protected]>

Quoted 8 lines

Quoted 5 lines
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.
This will break the existing gcc run time, which doesn't zero these arguments.

I'm a little lost here. Weren't these flags new in the as-yet-unreleased 3.19? How does gcc run-time depends on them already?

Thanks,

Michael

Quoted 28 lines

Quoted 24 lines

Signed-off-by Michael Kerrisk <[email protected]>

---
 kernel/sys.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/sys.c +4 -1

index a8c9f5a..ea9c881 100644
@@ -2210,9 +2210,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
              up_write(&me->mm->mmap_sem);
              break;
      case PR_MPX_ENABLE_MANAGEMENT:
+             if (arg2 || arg3 || arg4 || arg5)
+                     return -EINVAL;
              error = MPX_ENABLE_MANAGEMENT(me);
              break;
      case PR_MPX_DISABLE_MANAGEMENT:
+             if (arg2 || arg3 || arg4 || arg5)
+                     return -EINVAL;
              error = MPX_DISABLE_MANAGEMENT(me);
              break;
      default:
--

Quoted 1 line

1.9.3

--
[email protected] -- Speaking for myself only

Signature · 9 lines

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dave Hansen

2015-01-09 18:34 UTC #3 lore raw

Quoted 10 lines · Michael Kerrisk (man-pages)

Quoted 9 lines · Andi Kleen
Quoted 7 lines · Michael Kerrisk (man-pages)
From: Michael Kerrisk <[email protected]>
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.

Quoted 5 lines

Quoted 2 lines
This will break the existing gcc run time, which doesn't zero these arguments.
I'm a little lost here. Weren't these flags new in the as-yet-unreleased 3.19? How does gcc run-time depends on them already?

These prctl()s have been around in some form or another for a few months since the patches had not yet been merged in to the kernel. There is support for them in a set of (yet unmerged) gcc patches, as well as some tests which are only internal to Intel.

This change will, indeed, break those internal tests as well as the gcc patches. As far as I know, the code is not in production anywhere and can be changed. The prctl() numbers have changed while the patches were out of tree and it's a somewhat painful process each time it changes. It's not impossible, just painful.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael Kerrisk (man-pages)

author

2015-01-10 13:49 UTC #4 lore raw

Quoted 22 lines · Dave Hansen

Quoted 10 lines · Michael Kerrisk (man-pages)
Quoted 9 lines · Andi Kleen
Quoted 7 lines · Michael Kerrisk (man-pages)
From: Michael Kerrisk <[email protected]>
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.
Quoted 5 lines
Quoted 2 lines
This will break the existing gcc run time, which doesn't zero these arguments.
I'm a little lost here. Weren't these flags new in the as-yet-unreleased 3.19? How does gcc run-time depends on them already?
These prctl()s have been around in some form or another for a few months since the patches had not yet been merged in to the kernel. There is support for them in a set of (yet unmerged) gcc patches, as well as some tests which are only internal to Intel.

Quoted 5 lines

This change will, indeed, break those internal tests as well as the gcc patches. As far as I know, the code is not in production anywhere and can be changed. The prctl() numbers have changed while the patches were out of tree and it's a somewhat painful process each time it changes. It's not impossible, just painful.

So, sounds like thinks can be fixed (with mild inconvenience), and they should be fixed before 3.19 is actually released.

Cheers,

Michael

Signature · 9 lines

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andi Kleen

2015-01-10 18:39 UTC #5 lore raw

Quoted 23 lines · Michael Kerrisk (man-pages)

Quoted 22 lines · Dave Hansen
Quoted 10 lines · Michael Kerrisk (man-pages)
Quoted 9 lines · Andi Kleen
Quoted 7 lines · Michael Kerrisk (man-pages)
From: Michael Kerrisk <[email protected]>
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.
Quoted 5 lines
Quoted 2 lines
This will break the existing gcc run time, which doesn't zero these arguments.
I'm a little lost here. Weren't these flags new in the as-yet-unreleased 3.19? How does gcc run-time depends on them already?
These prctl()s have been around in some form or another for a few months since the patches had not yet been merged in to the kernel. There is support for them in a set of (yet unmerged) gcc patches, as well as some tests which are only internal to Intel.

Quoted 8 lines

Quoted 5 lines
This change will, indeed, break those internal tests as well as the gcc patches. As far as I know, the code is not in production anywhere and can be changed. The prctl() numbers have changed while the patches were out of tree and it's a somewhat painful process each time it changes. It's not impossible, just painful.
So, sounds like thinks can be fixed (with mild inconvenience), and they should be fixed before 3.19 is actually released.

FWIW I added these checks to prctl first, but in hindsight it was a mistake.

The glibc prctl() function is stdarg. Often you only have a single extra argument, so you need to add 4 zeroes. There is no compile time checking. It is very easy to get wrong and miscount the zeroes, happened several times. The failure may be hard to catch, because it only happens at runtime.

Also the extra zeroes look ugly in the source.

And it doesn't really buy you anything because it's very cheap to add new prctl numbers if you want to extend something.

So I would advise against it.

-Andi

Michael Kerrisk (man-pages)

author

2015-01-14 07:21 UTC #6 lore raw

Hi Andi,

Quoted 30 lines · Andi Kleen

Quoted 29 lines · Michael Kerrisk (man-pages)
Quoted 27 lines · Dave Hansen
Quoted 10 lines · Michael Kerrisk (man-pages)
Quoted 9 lines · Andi Kleen
Quoted 7 lines · Michael Kerrisk (man-pages)
From: Michael Kerrisk <[email protected]>
commit fe8c7f5cbf91124987106faa3bdf0c8b955c4cf7 added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure that unused arguments are zero, as is done in many existing prctl()s and as should be done for all new prctl()s. This patch adds the required checks.
Quoted 5 lines
Quoted 2 lines
This will break the existing gcc run time, which doesn't zero these arguments.
I'm a little lost here. Weren't these flags new in the as-yet-unreleased 3.19? How does gcc run-time depends on them already?
These prctl()s have been around in some form or another for a few months since the patches had not yet been merged in to the kernel. There is support for them in a set of (yet unmerged) gcc patches, as well as some tests which are only internal to Intel.
This change will, indeed, break those internal tests as well as the gcc patches. As far as I know, the code is not in production anywhere and can be changed. The prctl() numbers have changed while the patches were out of tree and it's a somewhat painful process each time it changes. It's not impossible, just painful.

Quoted 5 lines

Quoted 2 lines
So, sounds like thinks can be fixed (with mild inconvenience), and they should be fixed before 3.19 is actually released.
FWIW I added these checks to prctl first, but in hindsight it was a mistake.

(I'm not clear here whether you mean you added them for other prctl() operations, of for the MPX operations.)

Quoted 1 line

The glibc prctl() function is stdarg.

Sigh. Yes. It's ugly.

Quoted 2 lines

Often you only have a single
extra argument, so you need to add 4 zeroes.

And more ugliness. As far as I can tell, no prctl() operation even uses arg5. PR_SET_MM passes it to its helper routine, but that routine requires the argument to be zero. And PR_SET_MM is the only operation that uses arg4. That observation inclines me even more to a point I thought about recently: PR_SET_MM is sufficiently complicated that it really should have been a separate system call, rather than being crammed into prctl().

Carrying on with this point, the only operations that use arg3 are PR_MCE_KILL, PR_SET_MM, PR_SET_SECCOMP. And, after prodding from me and one or two others, the functionality of that last one was eventually split off into a separate seccomp() system call instead of further overloading prctl().

I know it's ancient history (prctl() dates pack to Linux 2.2 days), but one has to wonder what people were thinking of when they decided it was a good idea to add a generic multi-argument system call. (Even in the first operation that was added, PR_SET_PDEATHSIG, only arg2 was used.) Like, "hey ioctl() is a great idea, let's try the same idea with even more arguments".

Quoted 3 lines

There is no compile
time checking. It is very easy to get wrong and miscount the zeroes, happened several times.

Yes, but (in the case of those prctl() operations that don't check for the zeros), what's the harm of getting it wrong?

Quoted 2 lines

The failure may be hard to catch, because
it only happens at runtime.

<irony>
But, I mean, the developer will catch that on the first test, right? </irony>

Quoted 1 line

Also the extra zeroes look ugly in the source.

I don't suppose you can be proposing this as a strong argument, but I don''t think this pomit carries much wait at all. Anyway, the fundamental point is that it's the API that is ugly, not the zeros.

Quoted 2 lines

And it doesn't really buy you anything because it's very cheap to add new prctl numbers if you want to extend something.

I still tend to disagree. There's already enough completely unrelated functionality overloaded onto this API. I don't think we really want to follow the philosophy of "oh, we'll just add another operation later".

Quoted 1 line

So I would advise against it.

The counter-argument here is of course that user-space programmers sometimes make the converse error: omitting an argument on some prctl() that requires. The consequence of that sort of error can be considerably worse than miscounting the zeros required for prctl() operations that don't need check their unused arguments. Overall, given the messy API, I think it best to encourage user-space programmers into a discipline of always supplying the zeros for the unused arguments, so I still think this patch should be applied.

Cheers,

Michael

Signature · 9 lines

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>