lkml.org 
[lkml]   [2011]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [git pull] apparmor fix for __d_path() misuse
From
Date
Al Viro wrote:
> BTW, what your current code does if you have a file bound on another
> file, open it, umount -l it, let the dust settle and then do some operation
> that triggers tomoyo_get_absolute_path() on it? Because you'll be getting
> a vfsmount/dentry pair that has
> * dentry == vfsmount->mnt_root
> * vfsmount->mnt_parent == vfsmount
> * dentry->d_inode being a non-directory
> and there is nothing whatsoever in what remains of the pathname. Not a single
> component. IOW, you'll get "/" in buf. Might be good in a testsuite - is
> there any code in security/tomoyo that would be relying on assumption that
> only directory might have a name entirely without components?

TOMOYO assumes that only directory ends with '/'.

I wrote a test program and compared among below cases.

current code
current code + your patch
current code + your patch + my patch (attached at the bottom)

---------- test1.c ----------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#define MNT_DETACH 2

static void die(const char *msg)
{
fprintf(stderr, "%s\n", msg);
exit(1);
}

int main(int argc, char *argv[])
{
int fd;
if (mount("/tmp/", "/tmp", "tmpfs", 0, NULL))
die("Can't mount");
fd = open("/tmp/file1", O_WRONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't create /tmp/file1");
close(fd);
fd = open("/tmp/file2", O_WRONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't create /tmp/file2");
close(fd);
if (mount("/tmp/file1", "/tmp/file2", NULL, MS_BIND, 0))
die("Can't bind mount");
fd = open("/tmp/file2", O_RDONLY | O_CREAT, 0600);
if (fd == EOF)
die("Can't open /tmp/file2");
if (umount2("/tmp/file2", MNT_DETACH))
die("Can't unmount 2");
ioctl(fd, 0);
close(fd);
return 0;
}
---------- test1.c ----------

Below are the policy checked by TOMOYO.

---- linux b835c0f4 ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file ioctl / 0x0
3: file mount /tmp/ /tmp/ tmpfs 0x0
4: file mount /tmp/file1 /tmp/file2 --bind 0x0
5: file read /tmp/file2
6: file unmount /tmp/file2
7: file write /tmp/file1
8: file write /tmp/file2

ioctl() handled but its pathname is wrong.

---- linux b835c0f4 with your patch ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file mount /tmp/ /tmp/ tmpfs 0x0
3: file mount /tmp/file1 /tmp/file2 --bind 0x0
4: file read /tmp/file2
5: file unmount /tmp/file2
6: file write /tmp/file1
7: file write /tmp/file2

ioctl() failed (i.e. regression) because tomoyo_get_absolute_path() failed.
The error message was

ERROR: Out of memory at tomoyo_realpath_from_path.

---- linux b835c0f4 with your patch and my patch ----
0: file create /tmp/file1 0600
1: file create /tmp/file2 0600
2: file ioctl tmpfs:/file1 0x0
3: file mount /tmp/ /tmp/ tmpfs 0x0
4: file mount /tmp/file1 /tmp/file2 --bind 0x0
5: file read /tmp/file2
6: file unmount /tmp/file2
7: file write /tmp/file1
8: file write /tmp/file2

ioctl() handled using pathname returned by tomoyo_get_local_path().

> The real question is what pathname do you _want_ in this situation. Define
> that and we'll be able to do something about it;

Among above three results, the last one will be the best.

[PATCH] TOMOYO: Fix pathname handling of disconnected paths.

Current tomoyo_realpath_from_path() implementation returns strange pathname
when calculating pathname of a file which belongs to lazy unmounted tree.
Use local pathname rather than strange absolute pathname in that case.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -293,8 +293,16 @@ char *tomoyo_realpath_from_path(struct path *path)
pos = tomoyo_get_local_path(path->dentry, buf,
buf_len - 1);
/* Get absolute name for the rest. */
- else
+ else {
pos = tomoyo_get_absolute_path(path, buf, buf_len - 1);
+ /*
+ * Fall back to local name if absolute name is not
+ * available.
+ */
+ if (pos == ERR_PTR(-EINVAL))
+ pos = tomoyo_get_local_path(path->dentry, buf,
+ buf_len - 1);
+ }
encode:
if (IS_ERR(pos))
continue;

\
 
 \ /
  Last update: 2011-12-07 10:03    [W:0.111 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site