diff mbox series

proc: Track /proc/$pid/attr/ opener mm_struct

Message ID 20210608171221.276899-1-keescook@chromium.org
State Accepted
Commit 591a22c14d3f45cc38bd1931c593c221df2f1881
Headers show
Series proc: Track /proc/$pid/attr/ opener mm_struct | expand

Commit Message

Kees Cook June 8, 2021, 5:12 p.m. UTC
Commit bfb819ea20ce ("proc: Check /proc/$pid/attr/ writes against file opener")
tried to make sure that there could not be a confusion between the opener of
a /proc/$pid/attr/ file and the writer. It used struct cred to make sure
the privileges didn't change. However, there were existing cases where a more
privileged thread was passing the opened fd to a differently privileged thread
(during container setup). Instead, use mm_struct to track whether the opener
and writer are still the same process. (This is what several other proc files
already do, though for different reasons.)

Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
Reported-by: Andrea Righi <andrea.righi@canonical.com>
Tested-by: Andrea Righi <andrea.righi@canonical.com>
Fixes: bfb819ea20ce ("proc: Check /proc/$pid/attr/ writes against file opener")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/base.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

youling 257 June 14, 2021, 10:02 a.m. UTC | #1
I used mainline kernel on android, this patch cause "failed to retrieve pid context" problem.

06-14 02:15:51.165  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1682) failed to retrieve pid context.
06-14 02:15:51.166  1685  1685 E ServiceManager: add_service('batteryproperties',1) uid=0 - PERMISSION DENIED
06-14 02:15:51.166  1682  1682 I ServiceManager: addService() batteryproperties failed (err -1 - no service manager yet?).  Retrying...
06-14 02:15:51.197  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1695) failed to retrieve pid context.
06-14 02:15:51.197  1685  1685 E ServiceManager: add_service('android.security.keystore',1) uid=1017 - PERMISSION DENIED
06-14 02:15:51.198  1695  1695 I ServiceManager: addService() android.security.keystore failed (err -1 - no service manager yet?).  Retrying...
06-14 02:15:51.207  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1708) failed to retrieve pid context.
06-14 02:15:51.207  1685  1685 E ServiceManager: add_service('android.service.gatekeeper.IGateKeeperService',1) uid=1000 - PERMISSION DENIED
06-14 02:15:51.207  1708  1708 I ServiceManager: addService() android.service.gatekeeper.IGateKeeperService failed (err -1 - no service manager yet?).  Retrying...
06-14 02:15:51.275  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1693) failed to retrieve pid context.
06-14 02:15:51.275  1692  1692 I cameraserver: ServiceManager: 0xf6d309e0
06-14 02:15:51.275  1685  1685 E ServiceManager: add_service('drm.drmManager',1) uid=1019 - PERMISSION DENIED
06-14 02:15:51.276  1693  1693 I ServiceManager: addService() drm.drmManager failed (err -1 - no service manager yet?).  Retrying...
Kees Cook June 14, 2021, 3:32 p.m. UTC | #2
On Mon, Jun 14, 2021 at 06:02:34PM +0800, youling257 wrote:
> I used mainline kernel on android, this patch cause "failed to retrieve pid context" problem.

> 

> 06-14 02:15:51.165  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1682) failed to retrieve pid context.

> 06-14 02:15:51.166  1685  1685 E ServiceManager: add_service('batteryproperties',1) uid=0 - PERMISSION DENIED

> 06-14 02:15:51.166  1682  1682 I ServiceManager: addService() batteryproperties failed (err -1 - no service manager yet?).  Retrying...

> 06-14 02:15:51.197  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1695) failed to retrieve pid context.

> 06-14 02:15:51.197  1685  1685 E ServiceManager: add_service('android.security.keystore',1) uid=1017 - PERMISSION DENIED

> 06-14 02:15:51.198  1695  1695 I ServiceManager: addService() android.security.keystore failed (err -1 - no service manager yet?).  Retrying...

> 06-14 02:15:51.207  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1708) failed to retrieve pid context.

> 06-14 02:15:51.207  1685  1685 E ServiceManager: add_service('android.service.gatekeeper.IGateKeeperService',1) uid=1000 - PERMISSION DENIED

> 06-14 02:15:51.207  1708  1708 I ServiceManager: addService() android.service.gatekeeper.IGateKeeperService failed (err -1 - no service manager yet?).  Retrying...

> 06-14 02:15:51.275  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1693) failed to retrieve pid context.

> 06-14 02:15:51.275  1692  1692 I cameraserver: ServiceManager: 0xf6d309e0

> 06-14 02:15:51.275  1685  1685 E ServiceManager: add_service('drm.drmManager',1) uid=1019 - PERMISSION DENIED

> 06-14 02:15:51.276  1693  1693 I ServiceManager: addService() drm.drmManager failed (err -1 - no service manager yet?).  Retrying...

> 


Argh. Are you able to uncover what userspace is doing here?

So far, my test cases are:

1) self: open, write, close: allowed
2) self: open, clone thread. thread: change privileges, write, close: allowed
3) self: open, give to privileged process. privileged process: write: reject


-- 
Kees Cook
Kees Cook June 14, 2021, 4:45 p.m. UTC | #3
On Mon, Jun 14, 2021 at 08:32:35AM -0700, Kees Cook wrote:
> On Mon, Jun 14, 2021 at 06:02:34PM +0800, youling257 wrote:

> > I used mainline kernel on android, this patch cause "failed to retrieve pid context" problem.

> > 

> > 06-14 02:15:51.165  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1682) failed to retrieve pid context.


I found getpidcon() in libselinux:
https://github.com/SELinuxProject/selinux/blob/master/libselinux/src/procattr.c#L159

> > 06-14 02:15:51.166  1685  1685 E ServiceManager: add_service('batteryproperties',1) uid=0 - PERMISSION DENIED

> > 06-14 02:15:51.166  1682  1682 I ServiceManager: addService() batteryproperties failed (err -1 - no service manager yet?).  Retrying...

> > 06-14 02:15:51.197  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1695) failed to retrieve pid context.

> > 06-14 02:15:51.197  1685  1685 E ServiceManager: add_service('android.security.keystore',1) uid=1017 - PERMISSION DENIED

> > 06-14 02:15:51.198  1695  1695 I ServiceManager: addService() android.security.keystore failed (err -1 - no service manager yet?).  Retrying...

> > 06-14 02:15:51.207  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1708) failed to retrieve pid context.

> > 06-14 02:15:51.207  1685  1685 E ServiceManager: add_service('android.service.gatekeeper.IGateKeeperService',1) uid=1000 - PERMISSION DENIED

> > 06-14 02:15:51.207  1708  1708 I ServiceManager: addService() android.service.gatekeeper.IGateKeeperService failed (err -1 - no service manager yet?).  Retrying...

> > 06-14 02:15:51.275  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1693) failed to retrieve pid context.

> > 06-14 02:15:51.275  1692  1692 I cameraserver: ServiceManager: 0xf6d309e0

> > 06-14 02:15:51.275  1685  1685 E ServiceManager: add_service('drm.drmManager',1) uid=1019 - PERMISSION DENIED

> > 06-14 02:15:51.276  1693  1693 I ServiceManager: addService() drm.drmManager failed (err -1 - no service manager yet?).  Retrying...

> > 

> 

> Argh. Are you able to uncover what userspace is doing here?


It looks like this is a case of attempting to _read_ the attr file, and
the new opener check was requiring the opener/target relationship pass
the mm_access() checks, which is clearly too strict.

> So far, my test cases are:

> 

> 1) self: open, write, close: allowed

> 2) self: open, clone thread. thread: change privileges, write, close: allowed

> 3) self: open, give to privileged process. privileged process: write: reject


I've now added:

4) self: open privileged process's attr, read, close: allowed

Can folks please test this patch to double-check?


diff --git a/fs/proc/base.c b/fs/proc/base.c
index 7118ebe38fa6..7c55301674e0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2676,7 +2676,14 @@ static int proc_pident_readdir(struct file *file, struct dir_context *ctx,
 #ifdef CONFIG_SECURITY
 static int proc_pid_attr_open(struct inode *inode, struct file *file)
 {
-	return __mem_open(inode, file, PTRACE_MODE_READ_FSCREDS);
+	struct mm_struct *mm = __mem_open(inode, file, PTRACE_MODE_READ_FSCREDS);
+
+	/* Reads do not require mm_struct access. */
+	if (IS_ERR(mm))
+		mm = NULL;
+
+	file->private_data = mm;
+	return 0;
 }
 
 static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
@@ -2709,7 +2716,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	int rv;
 
 	/* A task may only write when it was the opener. */
-	if (file->private_data != current->mm)
+	if (!file->private_data || file->private_data != current->mm)
 		return -EPERM;
 
 	rcu_read_lock();


Wheee.

-- 
Kees Cook
Casey Schaufler June 14, 2021, 5:52 p.m. UTC | #4
On 6/14/2021 8:32 AM, Kees Cook wrote:
> On Mon, Jun 14, 2021 at 06:02:34PM +0800, youling257 wrote:

>> I used mainline kernel on android, this patch cause "failed to retrieve pid context" problem.

>>

>> 06-14 02:15:51.165  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1682) failed to retrieve pid context.

>> 06-14 02:15:51.166  1685  1685 E ServiceManager: add_service('batteryproperties',1) uid=0 - PERMISSION DENIED

>> 06-14 02:15:51.166  1682  1682 I ServiceManager: addService() batteryproperties failed (err -1 - no service manager yet?).  Retrying...

>> 06-14 02:15:51.197  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1695) failed to retrieve pid context.

>> 06-14 02:15:51.197  1685  1685 E ServiceManager: add_service('android.security.keystore',1) uid=1017 - PERMISSION DENIED

>> 06-14 02:15:51.198  1695  1695 I ServiceManager: addService() android.security.keystore failed (err -1 - no service manager yet?).  Retrying...

>> 06-14 02:15:51.207  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1708) failed to retrieve pid context.

>> 06-14 02:15:51.207  1685  1685 E ServiceManager: add_service('android.service.gatekeeper.IGateKeeperService',1) uid=1000 - PERMISSION DENIED

>> 06-14 02:15:51.207  1708  1708 I ServiceManager: addService() android.service.gatekeeper.IGateKeeperService failed (err -1 - no service manager yet?).  Retrying...

>> 06-14 02:15:51.275  1685  1685 E ServiceManager: SELinux: getpidcon(pid=1693) failed to retrieve pid context.

>> 06-14 02:15:51.275  1692  1692 I cameraserver: ServiceManager: 0xf6d309e0

>> 06-14 02:15:51.275  1685  1685 E ServiceManager: add_service('drm.drmManager',1) uid=1019 - PERMISSION DENIED

>> 06-14 02:15:51.276  1693  1693 I ServiceManager: addService() drm.drmManager failed (err -1 - no service manager yet?).  Retrying...

>>

> Argh. Are you able to uncover what userspace is doing here?

>

> So far, my test cases are:

>

> 1) self: open, write, close: allowed

> 2) self: open, clone thread. thread: change privileges, write, close: allowed

> 3) self: open, give to privileged process. privileged process: write: reject


I found an issue under Smack where a privileged process opened
/proc/self/attr/smack/current, wrote to it successfully, then tried
to write to it again, which failed because the cred has changed. 
That's not a common use case. The usual case is open, write, close.
If ServiceManager is assuming that it can leave a descriptor open
while manipulations are in progress it could encounter the same kind
of problem.
Linus Torvalds June 14, 2021, 6:02 p.m. UTC | #5
On Mon, Jun 14, 2021 at 9:45 AM Kees Cook <keescook@chromium.org> wrote:
>

>         /* A task may only write when it was the opener. */

> -       if (file->private_data != current->mm)

> +       if (!file->private_data || file->private_data != current->mm)


I don't think this is necessary.

If file->private_data is NULL, then the old test for private_data !=
current->mm will still work just fine.

Because if you can fool kernel threads to do the write for you, you
have bigger security issues than that test.

               Linus
youling 257 June 14, 2021, 6:46 p.m. UTC | #6
I test this patch cause "init: cannot setexeccon(u:r:ueventd:s0)
operation not permitted.
init ctrl_write_limited.

2021-06-15 0:45 GMT+08:00, Kees Cook <keescook@chromium.org>:
> On Mon, Jun 14, 2021 at 08:32:35AM -0700, Kees Cook wrote:

>> On Mon, Jun 14, 2021 at 06:02:34PM +0800, youling257 wrote:

>> > I used mainline kernel on android, this patch cause "failed to retrieve

>> > pid context" problem.

>> >

>> > 06-14 02:15:51.165  1685  1685 E ServiceManager: SELinux:

>> > getpidcon(pid=1682) failed to retrieve pid context.

>

> I found getpidcon() in libselinux:

> https://github.com/SELinuxProject/selinux/blob/master/libselinux/src/procattr.c#L159

>

>> > 06-14 02:15:51.166  1685  1685 E ServiceManager:

>> > add_service('batteryproperties',1) uid=0 - PERMISSION DENIED

>> > 06-14 02:15:51.166  1682  1682 I ServiceManager: addService()

>> > batteryproperties failed (err -1 - no service manager yet?).

>> > Retrying...

>> > 06-14 02:15:51.197  1685  1685 E ServiceManager: SELinux:

>> > getpidcon(pid=1695) failed to retrieve pid context.

>> > 06-14 02:15:51.197  1685  1685 E ServiceManager:

>> > add_service('android.security.keystore',1) uid=1017 - PERMISSION DENIED

>> > 06-14 02:15:51.198  1695  1695 I ServiceManager: addService()

>> > android.security.keystore failed (err -1 - no service manager yet?).

>> > Retrying...

>> > 06-14 02:15:51.207  1685  1685 E ServiceManager: SELinux:

>> > getpidcon(pid=1708) failed to retrieve pid context.

>> > 06-14 02:15:51.207  1685  1685 E ServiceManager:

>> > add_service('android.service.gatekeeper.IGateKeeperService',1) uid=1000

>> > - PERMISSION DENIED

>> > 06-14 02:15:51.207  1708  1708 I ServiceManager: addService()

>> > android.service.gatekeeper.IGateKeeperService failed (err -1 - no

>> > service manager yet?).  Retrying...

>> > 06-14 02:15:51.275  1685  1685 E ServiceManager: SELinux:

>> > getpidcon(pid=1693) failed to retrieve pid context.

>> > 06-14 02:15:51.275  1692  1692 I cameraserver: ServiceManager:

>> > 0xf6d309e0

>> > 06-14 02:15:51.275  1685  1685 E ServiceManager:

>> > add_service('drm.drmManager',1) uid=1019 - PERMISSION DENIED

>> > 06-14 02:15:51.276  1693  1693 I ServiceManager: addService()

>> > drm.drmManager failed (err -1 - no service manager yet?).  Retrying...

>> >

>>

>> Argh. Are you able to uncover what userspace is doing here?

>

> It looks like this is a case of attempting to _read_ the attr file, and

> the new opener check was requiring the opener/target relationship pass

> the mm_access() checks, which is clearly too strict.

>

>> So far, my test cases are:

>>

>> 1) self: open, write, close: allowed

>> 2) self: open, clone thread. thread: change privileges, write, close:

>> allowed

>> 3) self: open, give to privileged process. privileged process: write:

>> reject

>

> I've now added:

>

> 4) self: open privileged process's attr, read, close: allowed

>

> Can folks please test this patch to double-check?

>

>

> diff --git a/fs/proc/base.c b/fs/proc/base.c

> index 7118ebe38fa6..7c55301674e0 100644

> --- a/fs/proc/base.c

> +++ b/fs/proc/base.c

> @@ -2676,7 +2676,14 @@ static int proc_pident_readdir(struct file *file,

> struct dir_context *ctx,

>  #ifdef CONFIG_SECURITY

>  static int proc_pid_attr_open(struct inode *inode, struct file *file)

>  {

> -	return __mem_open(inode, file, PTRACE_MODE_READ_FSCREDS);

> +	struct mm_struct *mm = __mem_open(inode, file, PTRACE_MODE_READ_FSCREDS);

> +

> +	/* Reads do not require mm_struct access. */

> +	if (IS_ERR(mm))

> +		mm = NULL;

> +

> +	file->private_data = mm;

> +	return 0;

>  }

>

>  static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,

> @@ -2709,7 +2716,7 @@ static ssize_t proc_pid_attr_write(struct file * file,

> const char __user * buf,

>  	int rv;

>

>  	/* A task may only write when it was the opener. */

> -	if (file->private_data != current->mm)

> +	if (!file->private_data || file->private_data != current->mm)

>  		return -EPERM;

>

>  	rcu_read_lock();

>

>

> Wheee.

>

> --

> Kees Cook

>
Kees Cook June 14, 2021, 10:50 p.m. UTC | #7
On Tue, Jun 15, 2021 at 02:46:19AM +0800, youling 257 wrote:
> I test this patch cause "init: cannot setexeccon(u:r:ueventd:s0)

> operation not permitted.

> init ctrl_write_limited.


Thanks for testing!

This appears to come from here:
https://github.com/aosp-mirror/platform_system_core/blob/master/init/service.cpp#L242


In setexeccon(), I see (pid=0, attr="exec"):

        fd = openattr(pid, attr, O_RDWR | O_CLOEXEC);
...
                        ret = write(fd, context2, strlen(context2) + 1);
...
        close(fd);


and openattr() is doing:
...
                rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);
                if (rc < 0)
                        return -1;
                fd = open(path, flags | O_CLOEXEC);
...

I'm not sure how the above could fail. (mm_access() always allows
introspection...)

The only way I can understand the check failing is if a process did:

open, exec, write

But setexeccon() is not doing anything between the open and the write...

I will keep looking...

-Kees

-- 
Kees Cook
youling 257 June 15, 2021, 1:55 a.m. UTC | #8
if try to find problem on userspace, i used linux 5.13rc6 on old
android 7 cm14.1, not aosp android 11.
http://git.osdn.net/view?p=android-x86/system-core.git;a=blob;f=init/service.cpp;h=a5334f447fc2fc34453d2f6a37523bedccadc690;hb=refs/heads/cm-14.1-x86#l457

 457         if (!seclabel_.empty()) {
 458             if (setexeccon(seclabel_.c_str()) < 0) {
 459                 ERROR("cannot setexeccon('%s'): %s\n",
 460                       seclabel_.c_str(), strerror(errno));
 461                 _exit(127);
 462             }
 463         }

2021-06-15 6:50 GMT+08:00, Kees Cook <keescook@chromium.org>:
> On Tue, Jun 15, 2021 at 02:46:19AM +0800, youling 257 wrote:

>> I test this patch cause "init: cannot setexeccon(u:r:ueventd:s0)

>> operation not permitted.

>> init ctrl_write_limited.

>

> Thanks for testing!

>

> This appears to come from here:

> https://github.com/aosp-mirror/platform_system_core/blob/master/init/service.cpp#L242

>

>

> In setexeccon(), I see (pid=0, attr="exec"):

>

>         fd = openattr(pid, attr, O_RDWR | O_CLOEXEC);

> ...

>                         ret = write(fd, context2, strlen(context2) + 1);

> ...

>         close(fd);

>

>

> and openattr() is doing:

> ...

>                 rc = asprintf(&path, "/proc/thread-self/attr/%s", attr);

>                 if (rc < 0)

>                         return -1;

>                 fd = open(path, flags | O_CLOEXEC);

> ...

>

> I'm not sure how the above could fail. (mm_access() always allows

> introspection...)

>

> The only way I can understand the check failing is if a process did:

>

> open, exec, write

>

> But setexeccon() is not doing anything between the open and the write...

>

> I will keep looking...

>

> -Kees

>

> --

> Kees Cook

>
Linus Torvalds June 15, 2021, 6:19 p.m. UTC | #9
On Mon, Jun 14, 2021 at 6:55 PM youling 257 <youling257@gmail.com> wrote:
>

> if try to find problem on userspace, i used linux 5.13rc6 on old

> android 7 cm14.1, not aosp android 11.

> http://git.osdn.net/view?p=android-x86/system-core.git;a=blob;f=init/service.cpp;h=a5334f447fc2fc34453d2f6a37523bedccadc690;hb=refs/heads/cm-14.1-x86#l457

>

>  457         if (!seclabel_.empty()) {

>  458             if (setexeccon(seclabel_.c_str()) < 0) {

>  459                 ERROR("cannot setexeccon('%s'): %s\n",

>  460                       seclabel_.c_str(), strerror(errno));

>  461                 _exit(127);

>  462             }

>  463         }


I have no idea where the cm14.1 libraries are. Does anybody know where
the matching source code for setexeccon() would be?

For me - obviously not on cm14.1 - all "setexeccon()" does is

   n = openat(AT_FDCWD, "/proc/thread-self/attr/exec", O_RDWR|O_CLOEXEC)
   write(n, string, len)
   close(n)

and if that fails, it would seem to indicate that proc_mem_open()
failed. Which would be mm_access() failing. But I don't see how that
can be the case, because mm_access() explicitly allows "mm ==
current->mm" (which the above clearly should be).

youling, can you double-check with the current -git tree? But as far
as I can tell, my minimal patch is exactly the same as Kees' patch
(just smaller and simpler).

Kees, do you see anything?

           Linus
Kees Cook June 15, 2021, 9:50 p.m. UTC | #10
On Tue, Jun 15, 2021 at 11:19:04AM -0700, Linus Torvalds wrote:
> On Mon, Jun 14, 2021 at 6:55 PM youling 257 <youling257@gmail.com> wrote:

> >

> > if try to find problem on userspace, i used linux 5.13rc6 on old

> > android 7 cm14.1, not aosp android 11.

> > http://git.osdn.net/view?p=android-x86/system-core.git;a=blob;f=init/service.cpp;h=a5334f447fc2fc34453d2f6a37523bedccadc690;hb=refs/heads/cm-14.1-x86#l457

> >

> >  457         if (!seclabel_.empty()) {

> >  458             if (setexeccon(seclabel_.c_str()) < 0) {

> >  459                 ERROR("cannot setexeccon('%s'): %s\n",

> >  460                       seclabel_.c_str(), strerror(errno));

> >  461                 _exit(127);

> >  462             }

> >  463         }

> 

> I have no idea where the cm14.1 libraries are. Does anybody know where

> the matching source code for setexeccon() would be?

> 

> For me - obviously not on cm14.1 - all "setexeccon()" does is

> 

>    n = openat(AT_FDCWD, "/proc/thread-self/attr/exec", O_RDWR|O_CLOEXEC)

>    write(n, string, len)

>    close(n)

> 

> and if that fails, it would seem to indicate that proc_mem_open()

> failed. Which would be mm_access() failing. But I don't see how that

> can be the case, because mm_access() explicitly allows "mm ==

> current->mm" (which the above clearly should be).


Yeah, that was what I saw too.

> youling, can you double-check with the current -git tree? But as far

> as I can tell, my minimal patch is exactly the same as Kees' patch

> (just smaller and simpler).


FWIW, for that patch:

Acked-by: Kees Cook <keescook@chromium.org>

Cc: stable@vger.kernel.org

> 

> Kees, do you see anything?


No, I haven't been able to reproduce the failure. :(

-- 
Kees Cook
youling 257 June 16, 2021, 5:15 a.m. UTC | #11
I test "proc: only require mm_struct for writing" fixed my cm14.1 problem.

2021-06-16 2:19 GMT+08:00, Linus Torvalds <torvalds@linux-foundation.org>:
> On Mon, Jun 14, 2021 at 6:55 PM youling 257 <youling257@gmail.com> wrote:

>>

>> if try to find problem on userspace, i used linux 5.13rc6 on old

>> android 7 cm14.1, not aosp android 11.

>> http://git.osdn.net/view?p=android-x86/system-core.git;a=blob;f=init/service.cpp;h=a5334f447fc2fc34453d2f6a37523bedccadc690;hb=refs/heads/cm-14.1-x86#l457

>>

>>  457         if (!seclabel_.empty()) {

>>  458             if (setexeccon(seclabel_.c_str()) < 0) {

>>  459                 ERROR("cannot setexeccon('%s'): %s\n",

>>  460                       seclabel_.c_str(), strerror(errno));

>>  461                 _exit(127);

>>  462             }

>>  463         }

>

> I have no idea where the cm14.1 libraries are. Does anybody know where

> the matching source code for setexeccon() would be?

>

> For me - obviously not on cm14.1 - all "setexeccon()" does is

>

>    n = openat(AT_FDCWD, "/proc/thread-self/attr/exec", O_RDWR|O_CLOEXEC)

>    write(n, string, len)

>    close(n)

>

> and if that fails, it would seem to indicate that proc_mem_open()

> failed. Which would be mm_access() failing. But I don't see how that

> can be the case, because mm_access() explicitly allows "mm ==

> current->mm" (which the above clearly should be).

>

> youling, can you double-check with the current -git tree? But as far

> as I can tell, my minimal patch is exactly the same as Kees' patch

> (just smaller and simpler).

>

> Kees, do you see anything?

>

>            Linus

>
Greg KH June 16, 2021, 5:56 a.m. UTC | #12
On Tue, Jun 15, 2021 at 02:50:39PM -0700, Kees Cook wrote:
> On Tue, Jun 15, 2021 at 11:19:04AM -0700, Linus Torvalds wrote:

> > On Mon, Jun 14, 2021 at 6:55 PM youling 257 <youling257@gmail.com> wrote:

> > >

> > > if try to find problem on userspace, i used linux 5.13rc6 on old

> > > android 7 cm14.1, not aosp android 11.

> > > http://git.osdn.net/view?p=android-x86/system-core.git;a=blob;f=init/service.cpp;h=a5334f447fc2fc34453d2f6a37523bedccadc690;hb=refs/heads/cm-14.1-x86#l457

> > >

> > >  457         if (!seclabel_.empty()) {

> > >  458             if (setexeccon(seclabel_.c_str()) < 0) {

> > >  459                 ERROR("cannot setexeccon('%s'): %s\n",

> > >  460                       seclabel_.c_str(), strerror(errno));

> > >  461                 _exit(127);

> > >  462             }

> > >  463         }

> > 

> > I have no idea where the cm14.1 libraries are. Does anybody know where

> > the matching source code for setexeccon() would be?

> > 

> > For me - obviously not on cm14.1 - all "setexeccon()" does is

> > 

> >    n = openat(AT_FDCWD, "/proc/thread-self/attr/exec", O_RDWR|O_CLOEXEC)

> >    write(n, string, len)

> >    close(n)

> > 

> > and if that fails, it would seem to indicate that proc_mem_open()

> > failed. Which would be mm_access() failing. But I don't see how that

> > can be the case, because mm_access() explicitly allows "mm ==

> > current->mm" (which the above clearly should be).

> 

> Yeah, that was what I saw too.

> 

> > youling, can you double-check with the current -git tree? But as far

> > as I can tell, my minimal patch is exactly the same as Kees' patch

> > (just smaller and simpler).

> 

> FWIW, for that patch:

> 

> Acked-by: Kees Cook <keescook@chromium.org>

> Cc: stable@vger.kernel.org


Thanks, I'll go pick it up now.

greg k-h
diff mbox series

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 58bbf334265b..7118ebe38fa6 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2674,6 +2674,11 @@  static int proc_pident_readdir(struct file *file, struct dir_context *ctx,
 }
 
 #ifdef CONFIG_SECURITY
+static int proc_pid_attr_open(struct inode *inode, struct file *file)
+{
+	return __mem_open(inode, file, PTRACE_MODE_READ_FSCREDS);
+}
+
 static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
 				  size_t count, loff_t *ppos)
 {
@@ -2704,7 +2709,7 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 	int rv;
 
 	/* A task may only write when it was the opener. */
-	if (file->f_cred != current_real_cred())
+	if (file->private_data != current->mm)
 		return -EPERM;
 
 	rcu_read_lock();
@@ -2754,9 +2759,11 @@  static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 }
 
 static const struct file_operations proc_pid_attr_operations = {
+	.open		= proc_pid_attr_open,
 	.read		= proc_pid_attr_read,
 	.write		= proc_pid_attr_write,
 	.llseek		= generic_file_llseek,
+	.release	= mem_release,
 };
 
 #define LSM_DIR_OPS(LSM) \