[PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[Michael Kerrisk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 2480 bytes --]

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 */

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


--- linux-2.6.10-rc2/mm/mlock.c	2004-11-23 16:47:38.368147856 +0100
+++ linux-2.6.10-rc2-mlf/mm/mlock.c	2004-11-23 17:06:10.853024448 +0100
@@ -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!

Re: [PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[Andrew Morton]

"Michael Kerrisk" <mtk-lkml@gmx.net> wrote:
>
> 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().

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

--- 25/mm/mlock.c~rlimit_memlock-accounting-of-shmctl-shm_lock-is-broken	2004-11-23 08:58:13.299089928 -0800
+++ 25-akpm/mm/mlock.c	2004-11-23 09:01:31.500958664 -0800
@@ -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"?

Re: [PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[Michael Kerrisk]

> "Michael Kerrisk" <mtk-lkml@gmx.net> wrote:
> >
> > 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().
> 
> True.  We should make the same change to user_shm_unlock(), and we may as
> well tweak the excessive spinlock coverage in there too.

Thanks for the confirmation Andrew.

[...]

> 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"?

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 ++

Re: [PATCH 2.6.10-rc2] RLIMIT_MEMLOCK accounting of shmctl() SHM_LOCK is broken

[Hugh Dickins]

On Tue, 23 Nov 2004, Andrew Morton wrote:
> 
> True.  We should make the same change to user_shm_unlock(), and we may as
> well tweak the excessive spinlock coverage in there too.
> ...
> and then ask Hugh and Manfred to double-check.

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.

> 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"?

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

Further shmctl() SHM_LOCK strangeness

[Michael Kerrisk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 762 bytes --]

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!

Re: Further shmctl() SHM_LOCK strangeness

[Hugh Dickins]

On Wed, 24 Nov 2004, Michael Kerrisk wrote:
> 
> 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.

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

> 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.

Re: Further shmctl() SHM_LOCK strangeness

[Rik van Riel]

On Wed, 24 Nov 2004, Hugh Dickins wrote:

>> regardles of the segment's ownership or permissions,
>> providing the size of the segment falls within the
>> process's RLIMIT_MEMLOCK limit.

> 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?

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

Re: Further shmctl() SHM_LOCK strangeness

[Michael Kerrisk]

Rik,

> On Wed, 24 Nov 2004, Hugh Dickins wrote:
> 
> >> regardles of the segment's ownership or permissions,
> >> providing the size of the segment falls within the
> >> process's RLIMIT_MEMLOCK limit.
> 
> > 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?
> 
> Mlocking and munlocking of shm segments is accounted
> against the user_struct, not against the process.
> 
> This should stop any malicious exploits.

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 ++

Re: Further shmctl() SHM_LOCK strangeness

[Rik van Riel]

On Thu, 25 Nov 2004, Michael Kerrisk wrote:

> 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.

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

Re: Further shmctl() SHM_LOCK strangeness

[Michael Kerrisk]

Rik,

> On Thu, 25 Nov 2004, Michael Kerrisk wrote:
> 
> > 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.
> 
> 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

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

> or
> 
> 2) the process has the correct capabilities set

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 ++

Re: Further shmctl() SHM_LOCK strangeness

[Rik van Riel]

On Thu, 25 Nov 2004, Michael Kerrisk wrote:

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

Why?   We already have ulimits do that...

> 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.

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

Re: Further shmctl() SHM_LOCK strangeness

[Michael Kerrisk]

Rik,

> On Thu, 25 Nov 2004, Michael Kerrisk wrote:
> 
> > I don't think this is sufficient -- there must
> > be protection against arbitrary SHM_LOCKs.
> 
> Why?   We already have ulimits do that...

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.

> > 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.
> 
> It makes a lot of sense, but I don't know whether or not
> it'd break any applications...

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 ++

Re: Further shmctl() SHM_LOCK strangeness

[Michael Kerrisk]

Hugh,

> On Wed, 24 Nov 2004, Michael Kerrisk wrote:
> > 
> > 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.
> 
> That's a very good observation.

Thanks.

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

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

> 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?  

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.

> That would surely be wrong.

Agreed.  

Cheers,

Michael

> 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
> 
> > 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.
> 

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