[LTP] migrate_pages() of process with same UID in 4.15-rcX

[LTP] migrate_pages() of process with same UID in 4.15-rcX

[Jan Stancek]

Hi,

LTP test migrate_pages02 [1] is failing with 4.15-rcX, presumably as
consequence of:
  313674661925 "Unify migrate_pages and move_pages access checks"

The scenario is that privileged parent forks child, both parent
and child change euid to nobody and then parent tries to migrate
child to different node. Starting with 4.15-rcX it fails with EPERM.

Can anyone comment on accuracy of this sentence from man-pages
after commit 313674661925?

quoting man2/migrate_pages.2:
 "To move pages in another process, the caller must be privileged
 (CAP_SYS_NICE) or the real or effective user ID of the calling 
 process must match the real or saved-set user ID of the target
 process."

Thanks,
Jan

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c

migrate_pages() of process with same UID in 4.15-rcX

[Jan Stancek]

Hi,

LTP test migrate_pages02 [1] is failing with 4.15-rcX, presumably as
consequence of:
  313674661925 "Unify migrate_pages and move_pages access checks"

The scenario is that privileged parent forks child, both parent
and child change euid to nobody and then parent tries to migrate
child to different node. Starting with 4.15-rcX it fails with EPERM.

Can anyone comment on accuracy of this sentence from man-pages
after commit 313674661925?

quoting man2/migrate_pages.2:
 "To move pages in another process, the caller must be privileged
 (CAP_SYS_NICE) or the real or effective user ID of the calling 
 process must match the real or saved-set user ID of the target
 process."

Thanks,
Jan

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c

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

[LTP] migrate_pages() of process with same UID in 4.15-rcX

[Eric W. Biederman]

Jan Stancek <jstancek@redhat.com> writes:

> Hi,
>
> LTP test migrate_pages02 [1] is failing with 4.15-rcX, presumably as
> consequence of:
>   313674661925 "Unify migrate_pages and move_pages access checks"
>
> The scenario is that privileged parent forks child, both parent
> and child change euid to nobody and then parent tries to migrate
> child to different node. Starting with 4.15-rcX it fails with EPERM.
>
> Can anyone comment on accuracy of this sentence from man-pages
> after commit 313674661925?
>
> quoting man2/migrate_pages.2:
>  "To move pages in another process, the caller must be privileged
>  (CAP_SYS_NICE) or the real or effective user ID of the calling 
>  process must match the real or saved-set user ID of the target
>  process."
>
> Thanks,
> Jan
>
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c

The capability has been changed to CAP_SYS_PTRACE

The privilege check has been changed to the can you ptrace the other
process check (to avoid revealing how your pages are laid out to someone
who would not have known anyway) .  Which is essentially the same test
on uids.

*Scratches my head*

The code is using PTRACE_MODE_READ_REALCREDS which tests if the caller's
uid is the same and the target's uid, euid and suid.

The old code would test to see if either the caller's euid or
uid would match the targets uid or suid.  Which is extremely permissive.

For the LTP test above the fact that the target process does not have
matching uids looks like that will make it fail.


All of that said.  I am wondering if we should have used
PTRACE_MODE_READ_FSCREDS on these permission checks.  Using the caller's
euid would make more sense and if the comments are to be believed
PTRACE_MODE_READ_REALCREDS is only supposed to be used for backwards
compatibility.

Given that we can't be perfectly backwards compatibile I expect the
change should at least make sense.

AKA I think we should do:

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 4ce44d3ff03d..513f68020e9e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1404,7 +1404,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
         * Check if this process has the right to modify the specified process.
         * Use the regular "ptrace_may_access()" checks.
         */
-       if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
+       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
                rcu_read_unlock();
                err = -EPERM;
                goto out_put;
diff --git a/mm/migrate.c b/mm/migrate.c
index 4d0be47a322a..51124a0b63eb 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1776,7 +1776,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid, unsigned long, nr_pages,
         * Check if this process has the right to modify the specified
         * process. Use the regular "ptrace_may_access()" checks.
         */
-       if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
+       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
                rcu_read_unlock();
                err = -EPERM;
                goto out;


I know the LTP test case is not a regression and I know this won't fix
it.  I am just thinking since I am looking at it we should change the
permissions to something that makes more sense.

Eric

Re: migrate_pages() of process with same UID in 4.15-rcX

[Eric W. Biederman]

Jan Stancek <jstancek@redhat.com> writes:

> Hi,
>
> LTP test migrate_pages02 [1] is failing with 4.15-rcX, presumably as
> consequence of:
>   313674661925 "Unify migrate_pages and move_pages access checks"
>
> The scenario is that privileged parent forks child, both parent
> and child change euid to nobody and then parent tries to migrate
> child to different node. Starting with 4.15-rcX it fails with EPERM.
>
> Can anyone comment on accuracy of this sentence from man-pages
> after commit 313674661925?
>
> quoting man2/migrate_pages.2:
>  "To move pages in another process, the caller must be privileged
>  (CAP_SYS_NICE) or the real or effective user ID of the calling 
>  process must match the real or saved-set user ID of the target
>  process."
>
> Thanks,
> Jan
>
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/migrate_pages/migrate_pages02.c

The capability has been changed to CAP_SYS_PTRACE

The privilege check has been changed to the can you ptrace the other
process check (to avoid revealing how your pages are laid out to someone
who would not have known anyway) .  Which is essentially the same test
on uids.

*Scratches my head*

The code is using PTRACE_MODE_READ_REALCREDS which tests if the caller's
uid is the same and the target's uid, euid and suid.

The old code would test to see if either the caller's euid or
uid would match the targets uid or suid.  Which is extremely permissive.

For the LTP test above the fact that the target process does not have
matching uids looks like that will make it fail.


All of that said.  I am wondering if we should have used
PTRACE_MODE_READ_FSCREDS on these permission checks.  Using the caller's
euid would make more sense and if the comments are to be believed
PTRACE_MODE_READ_REALCREDS is only supposed to be used for backwards
compatibility.

Given that we can't be perfectly backwards compatibile I expect the
change should at least make sense.

AKA I think we should do:

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 4ce44d3ff03d..513f68020e9e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1404,7 +1404,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
         * Check if this process has the right to modify the specified process.
         * Use the regular "ptrace_may_access()" checks.
         */
-       if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
+       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
                rcu_read_unlock();
                err = -EPERM;
                goto out_put;
diff --git a/mm/migrate.c b/mm/migrate.c
index 4d0be47a322a..51124a0b63eb 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1776,7 +1776,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid, unsigned long, nr_pages,
         * Check if this process has the right to modify the specified
         * process. Use the regular "ptrace_may_access()" checks.
         */
-       if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
+       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
                rcu_read_unlock();
                err = -EPERM;
                goto out;


I know the LTP test case is not a regression and I know this won't fix
it.  I am just thinking since I am looking at it we should change the
permissions to something that makes more sense.

Eric





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

[LTP] migrate_pages() of process with same UID in 4.15-rcX

[Michal Hocko]

[Sorry for a very late reply]

On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
[...]
> All of that said.  I am wondering if we should have used
> PTRACE_MODE_READ_FSCREDS on these permission checks.

If this is really about preventing the layout discovery then we should
be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
Should we do the same thing here?
-- 
Michal Hocko
SUSE Labs

Re: migrate_pages() of process with same UID in 4.15-rcX

[Michal Hocko]

[Sorry for a very late reply]

On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
[...]
> All of that said.  I am wondering if we should have used
> PTRACE_MODE_READ_FSCREDS on these permission checks.

If this is really about preventing the layout discovery then we should
be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
Should we do the same thing here?
-- 
Michal Hocko
SUSE Labs

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

[LTP] migrate_pages() of process with same UID in 4.15-rcX

[Michal Hocko]

[Fixup Christoph email - the thread starts here
 http://lkml.kernel.org/r/1394749328.5225281.1515598510696.JavaMail.zimbra@redhat.com]

On Mon 29-01-18 14:31:51, Michal Hocko wrote:
> [Sorry for a very late reply]
> 
> On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
> [...]
> > All of that said.  I am wondering if we should have used
> > PTRACE_MODE_READ_FSCREDS on these permission checks.
> 
> If this is really about preventing the layout discovery then we should
> be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
> Should we do the same thing here?
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: migrate_pages() of process with same UID in 4.15-rcX

[Michal Hocko]

[Fixup Christoph email - the thread starts here
 http://lkml.kernel.org/r/1394749328.5225281.1515598510696.JavaMail.zimbra@redhat.com]

On Mon 29-01-18 14:31:51, Michal Hocko wrote:
> [Sorry for a very late reply]
> 
> On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
> [...]
> > All of that said.  I am wondering if we should have used
> > PTRACE_MODE_READ_FSCREDS on these permission checks.
> 
> If this is really about preventing the layout discovery then we should
> be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
> Should we do the same thing here?
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[LTP] migrate_pages() of process with same UID in 4.15-rcX

[Otto Ebeling]

Hi,

[sorry for the even later reply]

I don't have a strong preference either way (between fs creds or real 
creds), having the same behavior as proc_mem_open sounds like a sensible 
option too. Whether moving pages between NUMA nodes is a read-only 
(PTRACE_MODE_READ) activity is debatable, but I'm no NUMA expert.

My concern here was mainly about a) preventing layout discovery and b) 
consistency between move_pages and migrate_pages.

Otto



On Mon, 29 Jan 2018, Michal Hocko wrote:

> [Fixup Christoph email - the thread starts here
> http://lkml.kernel.org/r/1394749328.5225281.1515598510696.JavaMail.zimbra@redhat.com]
>
> On Mon 29-01-18 14:31:51, Michal Hocko wrote:
>> [Sorry for a very late reply]
>>
>> On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
>> [...]
>>> All of that said.  I am wondering if we should have used
>>> PTRACE_MODE_READ_FSCREDS on these permission checks.
>>
>> If this is really about preventing the layout discovery then we should
>> be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
>> Should we do the same thing here?
>> --
>> Michal Hocko
>> SUSE Labs
>
> -- 
> Michal Hocko
> SUSE Labs
>

Re: migrate_pages() of process with same UID in 4.15-rcX

[Otto Ebeling]

Hi,

[sorry for the even later reply]

I don't have a strong preference either way (between fs creds or real 
creds), having the same behavior as proc_mem_open sounds like a sensible 
option too. Whether moving pages between NUMA nodes is a read-only 
(PTRACE_MODE_READ) activity is debatable, but I'm no NUMA expert.

My concern here was mainly about a) preventing layout discovery and b) 
consistency between move_pages and migrate_pages.

Otto



On Mon, 29 Jan 2018, Michal Hocko wrote:

> [Fixup Christoph email - the thread starts here
> http://lkml.kernel.org/r/1394749328.5225281.1515598510696.JavaMail.zimbra@redhat.com]
>
> On Mon 29-01-18 14:31:51, Michal Hocko wrote:
>> [Sorry for a very late reply]
>>
>> On Wed 10-01-18 10:21:31, Eric W. Biederman wrote:
>> [...]
>>> All of that said.  I am wondering if we should have used
>>> PTRACE_MODE_READ_FSCREDS on these permission checks.
>>
>> If this is really about preventing the layout discovery then we should
>> be in sync with proc_mem_open and that uses PTRACE_MODE_FSCREDS|PTRACE_MODE_READ
>> Should we do the same thing here?
>> --
>> Michal Hocko
>> SUSE Labs
>
> -- 
> Michal Hocko
> SUSE Labs
>