RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

Linus, Andrew,

The accounting of shmctl() SHM_LOCK memory locks against the user structure is broken. The problem is that the check of the size of the to-be-locked region is based on the size of the segment as specified when it was created by shmget() (this size is *not* rounded up to a page boundary). This size is then rounded down (>> PAGE_SHIFT) to PAGE_SIZE during the check in mm/mlock.c::user_shm_lock().

The consequence is that one can lock an unlimited number of System V shared memory segments as long as they are each less than PAGE_SIZE bytes.

The program below can be used to demonstrate the problem. For example, running it with the following command line:

$ ./shm_lock_many 2000 1000

creates and locks 1000 segments of 2000 bytes each, even while RLIMIT_MEMLOCK is set to the default of 32kiB.

The simple patch at the end of this message is my attempt at a fix (it removes the problem when I test), but maybe there is something better.

Cheers,

Michael

=====================

/* shm_lock_many.c */

#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/shm.h>
#include <sys/stat.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>

#define errExit(msg) { perror(msg); exit(EXIT_FAILURE); }

int
main(int argc, char *argv[])

{
    int shmid, segSize, numSegs, j;

    if (argc < 3 || strcmp(argv[1], "--help") == 0) {
        fprintf(stderr, "Usage: %s segment-size num-segments\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    segSize = atoi(argv[1]);
    numSegs = atoi(argv[2]);

    for (j = 0; j < numSegs; j++) {
        shmid = shmget(IPC_PRIVATE, segSize, S_IRUSR | S_IWUSR);
        if (shmid == -1) errExit("shmget");
        printf("%d: %d\n", j, shmid);
        if (shmctl(shmid, SHM_LOCK, NULL) == -1) errExit("shmctl");
    }

    exit(EXIT_SUCCESS);
} /* main */

=======================

@@ -211,7 +211,7 @@
 	int allowed = 0;
 
 	spin_lock(&shmlock_user_lock);
-	locked = size >> PAGE_SHIFT;
+	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
 	lock_limit >>= PAGE_SHIFT;
 	if (locked + user->locked_shm > lock_limit && !capable(CAP_IPC_LOCK))

NEU +++ DSL Komplett von GMX +++ http://www.gmx.net/de/go/dsl
GMX DSL-Netzanschluss + Tarif zum supergünstigen Komplett-Preis!

True. We should make the same change to user_shm_unlock(), and we may as well tweak the excessive spinlock coverage in there too.

@@ -211,10 +211,10 @@ int user_shm_lock(size_t size, struct us
 	unsigned long lock_limit, locked;
 	int allowed = 0;
 
-	spin_lock(&shmlock_user_lock);
-	locked = size >> PAGE_SHIFT;
+	locked = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
 	lock_limit >>= PAGE_SHIFT;
+	spin_lock(&shmlock_user_lock);
 	if (locked + user->locked_shm > lock_limit && !capable(CAP_IPC_LOCK))
 		goto out;
 	get_uid(user);

@@ -228,7 +228,7 @@ out:
 void user_shm_unlock(size_t size, struct user_struct *user)
 {
 	spin_lock(&shmlock_user_lock);
-	user->locked_shm -= (size >> PAGE_SHIFT);
+	user->locked_shm -= (size + PAGE_SIZE - 1) >> PAGE_SHIFT);
 	spin_unlock(&shmlock_user_lock);
 	free_uid(user);
 }

_

and then ask Hugh and Manfred to double-check.

Looking at the callers, we do:

	user_shm_lock(inode->i_size, ...);

then, later:

	user_shm_unlock(inode->i_size, ...);

which does make one wonder "what happens if the file got larger while it was locked"?

Thanks for the confirmation Andrew.

[...]

Not sure if I'm missing your point, but, just considering it from the point of view of System V shared memory, the segment can't change in size after it has been created.

Cheers,

Michael

Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++

Looked good to me.

Examining that code for the first time, I did wonder about a couple of minor irrelevancies with regard to lock_limit - should it too be rounded up? Well, not necessarily, you can argue that the limit should be treated strictly. And it certainly shouldn't be rounded up (wrapping to 0) if it's RLIM_INFINITY. Which raises the question, should we avoid shifting it down if it's RLIM_INFINITY? And should there be a wrapping check on locked + user->locked_shm? Well, locking that much memory will meet its own problems, probably not worth worrying here.

They're only used on objects created by shmem_file_setup, and for those (unlike tmpfs files) there's no interface by which they might change their size after creation; and this isn't the only place which assumes that characteristic. So, it's okay (but not at all obvious).

Hugh

Manfred, Hugh,

While studying the RLIMIT_MEMLOCK stuff further, I came up with another observation: a process can perform a shmctl(SHM_LOCK) on *any* System V shared memory segment, regardles of the segment's ownership or permissions, providing the size of the segment falls within the process's RLIMIT_MEMLOCK limit.

Is this intended behaviour? For most other System V IPC "ctl" operations the process must either:

1. be the owner of the object or have an appropriate capability, or

2. have suitable permissions on the object.

Which of these two conditions applies depends on the "ctl" operation.

Cheers,

Michael

NEU +++ DSL Komplett von GMX +++ http://www.gmx.net/de/go/dsl
GMX DSL-Netzanschluss + Tarif zum supergünstigen Komplett-Preis!

That's a very good observation.

I think it's unintended, but I'm not sure. I've forgotten what can_do_mlock on shm was about.

Offhand I find it hard to grasp whether it's harmless or bad, but inclined to think bad - if there happen to be lots of small enough shared memory segments on the system, a series of processes run by one unprivileged user can lock down lots of memory?

Isn't it further the case that any process can now SHM_UNLOCK any segment? That would surely be wrong.

I've added Rik and Chris to the CC list, they seem to be the main can_do_mlock guys, hope they can answer.

Hugh

Mlocking and munlocking of shm segments is accounted against the user_struct, not against the process.

This should stop any malicious exploits.

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Rik,

As noted by Hugh, the problem also applies for
SHM_UNLOCK: anyone can unlock any System V shared memory segment. If our reason for locking memory was security (no swapping), then this does allow for exploits.

(Also, I just want to reemphasise that these semantics are inconsistent with the types of ownership and permission checking performed for just about every other kind of System V "ctl" operation.)

Cheers,

Michael

Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++

Good point. I guess that for SHM_UNLOCK operations we need to check for either:

1) current->user is the same user who SHM_LOCKed the segment in question

or

2) the process has the correct capabilities set

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Rik,

I don't think this is sufficient -- there must
be protection against arbitrary SHM_LOCKs.

How about the following:

For *both* SHM_LOCK and SHM_UNLOCK, the process should either be the owner or the creator of the object or have the CAP_IPC_LOCK capability.

Note the following:

1. SHM_LOCK should be covered so that a process with a high RLIMIT_MEMLOCK is allowed to lock arbitrary segments that it doesn't own.

2. A framework like the above is consistent with the semantics of the existing shmctl() IPC_SET and IPC_RMID operations (see the shmctl(2) man page).

Cheers,

Michael

Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++

Why? We already have ulimits do that...

It makes a lot of sense, but I don't know whether or not it'd break any applications...

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

Rik,

My gut feeling is that processes should not be able to arbitrarily lock shared memory segments created by other users in memory. I mean: why should I be able to someone else's segment into shared memory when I can't even access the contents of that shared memory. (Such semantics are simply inconsistent with the System V IPC model.)

Also (more below), I don't see any other sensible semantics for these operations, other than the ones I've proposed.

There is no reason why it should. In 2.6.8, the only processes that could lock shared memory segments were those with CAP_IPC_LOCK. Unprivileged processes did not get a look in.

2.6.9 changed things, but it is very unlikely that any applications depend on this (yet). Most userland developers are probably not even aware of the changed semantics in 2.6.9. The time to repair these semantics is *now*, before someone does depend on them. (In any case changes are required, since at a minimum, SHM_UNLOCK must be repaired.)

You earlier suggested the idea that SHM_UNLOCK might check to see if the process's user ID matched that of the process that did the SHM_LOCK. This doesn't work. Suppose someone else locks *my* segment (even though they don't have permission to access its contents or perform other "ctl" operations on it like IPC_RMID). Under your idea, I would not be able to do a SHM_UNLOCK to remove that lock.

Cheers,

Michael

Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++

Hugh,

Thanks.

can_do_mlock() returns true if we have CAP_IPC_LOCK or RLIMIT_MEMLOCK != 0.

I hadn't got as far as thinking about that, but you are correct. Anyone can SHM_UNLOCK a System V shared memory segment on 2.6.9, regardless of ownership and permissions.

Agreed.

Cheers,

Michael

Geschenkt: 3 Monate GMX ProMail + 3 Top-Spielfilme auf DVD
++ Jetzt kostenlos testen http://www.gmx.net/de/go/mail ++