LinuxLists.cc - [PATCH 00/23] Removal of binary sysctl support

2009-11-08 12:20:56

Subject: [PATCH 00/23] Removal of binary sysctl support

2009-11-08 13:15:45

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Hello.

Eric W. Biederman wrote:
>
> This patchset reimplements sys_sysctl as a compatibility wrapper
> around /proc/sys. After which it removes all of the code to all over
> the kernel that is used today to implement the binary sysctls.
>
> I am posting this patchset to give everyone a heads up what is in
> flight.
>
> I intend to carry all of these patches in my sysctl tree.
>
> If you add new sysctls to other trees please don't set the .ctl_name
> or .strategy fields in struct ctl_table, as setting those fields
> is unnecessary now and are removed by this patchset.

I have two questions.

Is there (or was there) an entry with ->procname == NULL ? (I thought so
because the loop condition is

for ( ; table->ctl_name || table->procname; table++) {

.)

This patchset removes ->ctl_name on the assumption that ->procname != NULL ?

Regards.

2009-11-08 23:39:23

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Tetsuo Handa <[email protected]> writes:

> Hello.
>
> Eric W. Biederman wrote:
>>
>> This patchset reimplements sys_sysctl as a compatibility wrapper
>> around /proc/sys. After which it removes all of the code to all over
>> the kernel that is used today to implement the binary sysctls.
>>
>> I am posting this patchset to give everyone a heads up what is in
>> flight.
>>
>> I intend to carry all of these patches in my sysctl tree.
>>
>> If you add new sysctls to other trees please don't set the .ctl_name
>> or .strategy fields in struct ctl_table, as setting those fields
>> is unnecessary now and are removed by this patchset.
>
> I have two questions.
>
> Is there (or was there) an entry with ->procname == NULL ? (I thought so
> because the loop condition is
>
> for ( ; table->ctl_name || table->procname; table++) {
>
> .)
>
> This patchset removes ->ctl_name on the assumption that ->procname != NULL ?

There has been a gradual transition from the assumption that the table ends with
!ctl_name to the assumption that procname == NULL. There is no sysctl entry
with a valid ctl_name without a valid procname.

Eric

2009-11-09 00:12:14

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Eric W. Biederman wrote:
> There has been a gradual transition from the assumption that the table ends with
> !ctl_name to the assumption that procname == NULL. There is no sysctl entry
> with a valid ctl_name without a valid procname.

I see. Then, please add below one to your patchset.

Regards.
----------
[PATCH] sysctl security/tomoyo: Don't look at ctl_name

ctl_name field was removed. Always use procname field.

Signed-off-by: Tetsuo Handa <[email protected]>
---
security/tomoyo/tomoyo.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)

--- security-testing-2.6.orig/security/tomoyo/tomoyo.c
+++ security-testing-2.6/security/tomoyo/tomoyo.c
@@ -122,15 +122,7 @@ static char *tomoyo_sysctl_path(struct c
*--end = '\0';
buflen--;
while (table) {
- char num[32];
- const char *sp = table->procname;
-
- if (!sp) {
- memset(num, 0, sizeof(num));
- snprintf(num, sizeof(num) - 1, "=%d=", table->ctl_name);
- sp = num;
- }
- if (tomoyo_prepend(&end, &buflen, sp) ||
+ if (tomoyo_prepend(&end, &buflen, table->procname) ||
tomoyo_prepend(&end, &buflen, "/"))
goto out;
table = table->parent;

2009-11-09 00:35:50

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

2009-11-18 18:44:20

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Tetsuo Handa <[email protected]> writes:

> Eric W. Biederman wrote:
>> There has been a gradual transition from the assumption that the table ends with
>> !ctl_name to the assumption that procname == NULL. There is no sysctl entry
>> with a valid ctl_name without a valid procname.
>
> I see. Then, please add below one to your patchset.

I have been looking at this and in the sysctl tree I am now going through
the vfs for all of the the operations on /proc/sys. I believe that means
we can completely remove the sysctl special case in tomoyo. Like I have
in the patch below.

Will that work?

Eric

diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 3f93bb9..8a00ade 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -85,75 +85,6 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
return tomoyo_check_open_permission(domain, &bprm->file->f_path, 1);
}

-#ifdef CONFIG_SYSCTL
-
-static int tomoyo_prepend(char **buffer, int *buflen, const char *str)
-{
- int namelen = strlen(str);
-
- if (*buflen < namelen)
- return -ENOMEM;
- *buflen -= namelen;
- *buffer -= namelen;
- memcpy(*buffer, str, namelen);
- return 0;
-}
-
-/**
- * tomoyo_sysctl_path - return the realpath of a ctl_table.
- * @table: pointer to "struct ctl_table".
- *
- * Returns realpath(3) of the @table on success.
- * Returns NULL on failure.
- *
- * This function uses tomoyo_alloc(), so the caller must call tomoyo_free()
- * if this function didn't return NULL.
- */
-static char *tomoyo_sysctl_path(struct ctl_table *table)
-{
- int buflen = TOMOYO_MAX_PATHNAME_LEN;
- char *buf = tomoyo_alloc(buflen);
- char *end = buf + buflen;
- int error = -ENOMEM;
-
- if (!buf)
- return NULL;
-
- *--end = '\0';
- buflen--;
- while (table) {
- if (tomoyo_prepend(&end, &buflen, table->procname) ||
- tomoyo_prepend(&end, &buflen, "/"))
- goto out;
- table = table->parent;
- }
- if (tomoyo_prepend(&end, &buflen, "/proc/sys"))
- goto out;
- error = tomoyo_encode(buf, end - buf, end);
- out:
- if (!error)
- return buf;
- tomoyo_free(buf);
- return NULL;
-}
-
-static int tomoyo_sysctl(struct ctl_table *table, int op)
-{
- int error;
- char *name;
-
- op &= MAY_READ | MAY_WRITE;
- if (!op)
- return 0;
- name = tomoyo_sysctl_path(table);
- if (!name)
- return -ENOMEM;
- error = tomoyo_check_file_perm(tomoyo_domain(), name, op);
- tomoyo_free(name);
- return error;
-}
-#endif
-
static int tomoyo_path_truncate(struct path *path, loff_t length,
unsigned int time_attrs)
{
@@ -274,9 +205,6 @@ static struct security_operations tomoyo_security_ops = {
.cred_transfer = tomoyo_cred_transfer,
.bprm_set_creds = tomoyo_bprm_set_creds,
.bprm_check_security = tomoyo_bprm_check_security,
-#ifdef CONFIG_SYSCTL
- .sysctl = tomoyo_sysctl,
-#endif
.file_fcntl = tomoyo_file_fcntl,
.dentry_open = tomoyo_dentry_open,
.path_truncate = tomoyo_path_truncate,

2009-11-18 22:04:17

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Hello.

Eric W. Biederman wrote:
> Tetsuo Handa writes:
>
> > Eric W. Biederman wrote:
> >> There has been a gradual transition from the assumption that the table ends with
> >> !ctl_name to the assumption that procname == NULL. There is no sysctl entry
> >> with a valid ctl_name without a valid procname.
> >
> > I see. Then, please add below one to your patchset.
>
> I have been looking at this and in the sysctl tree I am now going through
> the vfs for all of the the operations on /proc/sys. I believe that means
> we can completely remove the sysctl special case in tomoyo. Like I have
> in the patch below.
>
> Will that work?
>
> Eric

If you remove sysctl(2) from kernel and let userland libraries emulate

static int name[] = { CTL_NET, NET_IPV4, NET_IPV4_LOCAL_PORT_RANGE };
int buffer[2] = { 0, 0 };
int size = sizeof(buffer);
sysctl(name, 3, buffer, &size, 0, 0);

like

FILE *fp = fopen("/proc/sys/net/ipv4/ip_local_port_range", "r");
int buffer[2] = { 0, 0 };
fscanf(fp, "%u %u", &buffer[0], &buffer[1]);
fclose(fp);

or you modify sysctl(2) to call security_dentry_open() rather than
security_sysctl(), we can completely remove the sysctl special case in tomoyo.

Regards.

2009-11-18 22:45:19

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Tetsuo Handa <[email protected]> writes:

> Hello.
>
> Eric W. Biederman wrote:
>> Tetsuo Handa writes:
>>
>> > Eric W. Biederman wrote:
>> >> There has been a gradual transition from the assumption that the table ends with
>> >> !ctl_name to the assumption that procname == NULL. There is no sysctl entry
>> >> with a valid ctl_name without a valid procname.
>> >
>> > I see. Then, please add below one to your patchset.
>>
>> I have been looking at this and in the sysctl tree I am now going through
>> the vfs for all of the the operations on /proc/sys. I believe that means
>> we can completely remove the sysctl special case in tomoyo. Like I have
>> in the patch below.
>>
>> Will that work?
>>
>> Eric
>
> If you remove sysctl(2) from kernel and let userland libraries emulate
>
> static int name[] = { CTL_NET, NET_IPV4, NET_IPV4_LOCAL_PORT_RANGE };
> int buffer[2] = { 0, 0 };
> int size = sizeof(buffer);
> sysctl(name, 3, buffer, &size, 0, 0);
>
> like
>
> FILE *fp = fopen("/proc/sys/net/ipv4/ip_local_port_range", "r");
> int buffer[2] = { 0, 0 };
> fscanf(fp, "%u %u", &buffer[0], &buffer[1]);
> fclose(fp);
>
> or you modify sysctl(2) to call security_dentry_open() rather than
> security_sysctl(), we can completely remove the sysctl special case in tomoyo.

I have done something very close, the emulation is in the kernel not
user space, but the idea is the same.

The relevant bits of binary_sysctl() (from my sysctl tree) are:
mnt = current->nsproxy->pid_ns->proc_mnt;
result = vfs_path_lookup(mnt->mnt_root, mnt, pathname, 0, &nd);
if (result)
goto out_putname;

result = may_open(&nd.path, acc_mode, fmode);
if (result)
goto out_putpath;

file = dentry_open(nd.path.dentry, nd.path.mnt, flags, current_cred());
result = PTR_ERR(file);
if (IS_ERR(file))
goto out_putname;

dentry_open calls __dentry_open which calls security_dentry_open.

The twist that may get this into trouble is that I am going through
the internal vfs mount of /proc instead of the normal mount of proc.
So you will see paths like "/sys/net/ipv4/ip_local_port_range" instead
of "/proc/sys/net/ipv4/ip_local_port_range". I don't know how the
choice of mount points affects you.

Eric

2009-11-19 14:33:59

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Hello.

Eric W. Biederman wrote:
> Tetsuo Handa writes:
>
> > Hello.
> >
> > Eric W. Biederman wrote:
> >> Tetsuo Handa writes:
> >>
> >> > Eric W. Biederman wrote:
> >> >> There has been a gradual transition from the assumption that the table ends with
> >> >> !ctl_name to the assumption that procname == NULL. There is no sysctl entry
> >> >> with a valid ctl_name without a valid procname.
> >> >
> >> > I see. Then, please add below one to your patchset.
> >>
> >> I have been looking at this and in the sysctl tree I am now going through
> >> the vfs for all of the the operations on /proc/sys. I believe that means
> >> we can completely remove the sysctl special case in tomoyo. Like I have
> >> in the patch below.
> >>
> >> Will that work?
> >>
> >> Eric
> >
> > If you remove sysctl(2) from kernel and let userland libraries emulate
> >
> > static int name[] = { CTL_NET, NET_IPV4, NET_IPV4_LOCAL_PORT_RANGE };
> > int buffer[2] = { 0, 0 };
> > int size = sizeof(buffer);
> > sysctl(name, 3, buffer, &size, 0, 0);
> >
> > like
> >
> > FILE *fp = fopen("/proc/sys/net/ipv4/ip_local_port_range", "r");
> > int buffer[2] = { 0, 0 };
> > fscanf(fp, "%u %u", &buffer[0], &buffer[1]);
> > fclose(fp);
> >
> > or you modify sysctl(2) to call security_dentry_open() rather than
> > security_sysctl(), we can completely remove the sysctl special case in tomoyo.
>
> I have done something very close, the emulation is in the kernel not
> user space, but the idea is the same.
>
> The relevant bits of binary_sysctl() (from my sysctl tree) are:
> mnt = current->nsproxy->pid_ns->proc_mnt;
> result = vfs_path_lookup(mnt->mnt_root, mnt, pathname, 0, &nd);
> if (result)
> goto out_putname;
>
> result = may_open(&nd.path, acc_mode, fmode);
> if (result)
> goto out_putpath;
>
> file = dentry_open(nd.path.dentry, nd.path.mnt, flags, current_cred());
> result = PTR_ERR(file);
> if (IS_ERR(file))
> goto out_putname;
>
> dentry_open calls __dentry_open which calls security_dentry_open.
>

I see. We can remove the sysctl special case in tomoyo.

> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 3f93bb9..8a00ade 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -85,75 +85,6 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm)
> return tomoyo_check_open_permission(domain, &bprm->file->f_path, 1);
> }
>
> -#ifdef CONFIG_SYSCTL
> -
> -static int tomoyo_prepend(char **buffer, int *buflen, const char *str)
> -{
> - int namelen = strlen(str);
> -
> - if (*buflen < namelen)
> - return -ENOMEM;
> - *buflen -= namelen;
> - *buffer -= namelen;
> - memcpy(*buffer, str, namelen);
> - return 0;
> -}
> -
> -/**
> - * tomoyo_sysctl_path - return the realpath of a ctl_table.
> - * @table: pointer to "struct ctl_table".
> - *
> - * Returns realpath(3) of the @table on success.
> - * Returns NULL on failure.
> - *
> - * This function uses tomoyo_alloc(), so the caller must call tomoyo_free()
> - * if this function didn't return NULL.
> - */
> -static char *tomoyo_sysctl_path(struct ctl_table *table)
> -{
> - int buflen = TOMOYO_MAX_PATHNAME_LEN;
> - char *buf = tomoyo_alloc(buflen);
> - char *end = buf + buflen;
> - int error = -ENOMEM;
> -
> - if (!buf)
> - return NULL;
> -
> - *--end = '\0';
> - buflen--;
> - while (table) {
> - if (tomoyo_prepend(&end, &buflen, table->procname) ||
> - tomoyo_prepend(&end, &buflen, "/"))
> - goto out;
> - table = table->parent;
> - }
> - if (tomoyo_prepend(&end, &buflen, "/proc/sys"))
> - goto out;
> - error = tomoyo_encode(buf, end - buf, end);
> - out:
> - if (!error)
> - return buf;
> - tomoyo_free(buf);
> - return NULL;
> -}
> -
> -static int tomoyo_sysctl(struct ctl_table *table, int op)
> -{
> - int error;
> - char *name;
> -
> - op &= MAY_READ | MAY_WRITE;
> - if (!op)
> - return 0;
> - name = tomoyo_sysctl_path(table);
> - if (!name)
> - return -ENOMEM;
> - error = tomoyo_check_file_perm(tomoyo_domain(), name, op);
> - tomoyo_free(name);
> - return error;
> -}
> -#endif
> -
> static int tomoyo_path_truncate(struct path *path, loff_t length,
> unsigned int time_attrs)
> {
> @@ -274,9 +205,6 @@ static struct security_operations tomoyo_security_ops = {
> .cred_transfer = tomoyo_cred_transfer,
> .bprm_set_creds = tomoyo_bprm_set_creds,
> .bprm_check_security = tomoyo_bprm_check_security,
> -#ifdef CONFIG_SYSCTL
> - .sysctl = tomoyo_sysctl,
> -#endif
> .file_fcntl = tomoyo_file_fcntl,
> .dentry_open = tomoyo_dentry_open,
> .path_truncate = tomoyo_path_truncate,
>

Acked-by: Tetsuo Handa <[email protected]>

But please wait a bit. We need to solve the twist below.

> The twist that may get this into trouble is that I am going through
> the internal vfs mount of /proc instead of the normal mount of proc.
> So you will see paths like "/sys/net/ipv4/ip_local_port_range" instead
> of "/proc/sys/net/ipv4/ip_local_port_range". I don't know how the
> choice of mount points affects you.
>
> Eric
>

Indeed. TOMOYO and AppArmor need a hint for prepending "/proc" prefix.
A simple implementation which adds one bit to task_struct is shown below.
In this way, not only the file permission checks inside dentry_open()
but also the directory permission checks inside vfs_path_lookup() can be
prepended "/proc" prefix. AppArmor might want to prepend "/proc" inside
vfs_path_lookup().

Regards.
----------------------------------------
[PATCH 1/2] sysctl: Add in_sysctl flag to task_struct.

Pathname based access control prepends "/proc" prefix to the pathname obtained
by traversing ctl_table tree when binary sysctl is requested.

Now, binary sysctl code was rewritten to use internal vfs mount of /proc but
currently there is no hint which can give pathname based access control a
chance to prepend "/proc" prefix.

We want a hint which gives TOMOYO a chance to prepend "/proc" prefix.
There are two ways to implement this hint. One is to add 1 bit to task_struct
which remembers whether the current process is doing binary_sysctl() or not.
The other is to pass that bit to dentry_open(), security_dentry_open(),
tomoyo_dentry_open(), tomoyo_check_open_permission(), tomoyo_get_path(),
tomoyo_get_path() and tomoyo_realpath_from_path2().

This patch implements the former, for different LSM modules might want to

(a) prepend "/proc" prefix for checking directory permission inside
vfs_path_lookup()

or

(b) omit checking directory permission inside vfs_path_lookup()

Signed-off-by: Tetsuo Handa <[email protected]>
---
include/linux/sched.h | 2 ++
kernel/sysctl_binary.c | 2 ++
2 files changed, 4 insertions(+)

--- sysctl-2.6.orig/include/linux/sched.h
+++ sysctl-2.6/include/linux/sched.h
@@ -1279,6 +1279,8 @@ struct task_struct {
unsigned did_exec:1;
unsigned in_execve:1; /* Tell the LSMs that the process is doing an
* execve */
+ unsigned in_sysctl:1; /* Tell the LSMs that the process is doing a
+ * binary sysctl */
unsigned in_iowait:1;

--- sysctl-2.6.orig/kernel/sysctl_binary.c
+++ sysctl-2.6/kernel/sysctl_binary.c
@@ -1356,6 +1356,7 @@ static ssize_t binary_sysctl(const int *
goto out_putname;
}

+ current->in_sysctl = 1;
mnt = current->nsproxy->pid_ns->proc_mnt;
result = vfs_path_lookup(mnt->mnt_root, mnt, pathname, 0, &nd);
if (result)
@@ -1374,6 +1375,7 @@ static ssize_t binary_sysctl(const int *

fput(file);
out_putname:
+ current->in_sysctl = 0;
putname(pathname);
out:
return result;

----------------------------------------
[PATCH 1/2] TOMOYO: prepend /proc prefix for binary sysctl.

The pathname obtained by binary_sysctl() starts with "/sys".
This patch prepends "/proc" prefix if the pathname was obtained inside
binary_sysctl() so that TOMOYO checks a pathname which starts with "/proc/sys".

Signed-off-by: Tetsuo Handa <[email protected]>
---
security/tomoyo/realpath.c | 8 ++++++++
1 file changed, 8 insertions(+)

--- sysctl-2.6.orig/security/tomoyo/realpath.c
+++ sysctl-2.6/security/tomoyo/realpath.c
@@ -108,6 +108,14 @@ int tomoyo_realpath_from_path2(struct pa
spin_unlock(&dcache_lock);
path_put(&root);
path_put(&ns_root);
+ /* Prepend "/proc" prefix if binary_sysctl(). */
+ if (!IS_ERR(sp) && current->in_sysctl) {
+ sp -= 5;
+ if (sp >= newname)
+ memcpy(sp, "/proc", 5);
+ else
+ sp = ERR_PTR(-ENOMEM);
+ }
}
if (IS_ERR(sp))
error = PTR_ERR(sp);

2009-11-19 17:49:29

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Tetsuo Handa <[email protected]> writes:

> Acked-by: Tetsuo Handa <[email protected]>
>
> But please wait a bit. We need to solve the twist below.

Agreed.

> Indeed. TOMOYO and AppArmor need a hint for prepending "/proc" prefix.
> A simple implementation which adds one bit to task_struct is shown below.
> In this way, not only the file permission checks inside dentry_open()
> but also the directory permission checks inside vfs_path_lookup() can be
> prepended "/proc" prefix. AppArmor might want to prepend "/proc" inside
> vfs_path_lookup().

There don't appear to be any security hooks in vfs_path_lookup().
>
> Regards.
> ----------------------------------------
> [PATCH 1/2] sysctl: Add in_sysctl flag to task_struct.
>
> Pathname based access control prepends "/proc" prefix to the pathname obtained
> by traversing ctl_table tree when binary sysctl is requested.
>
> Now, binary sysctl code was rewritten to use internal vfs mount of /proc but
> currently there is no hint which can give pathname based access control a
> chance to prepend "/proc" prefix.

Actually there is.

> [PATCH 1/2] TOMOYO: prepend /proc prefix for binary sysctl.
>
> The pathname obtained by binary_sysctl() starts with "/sys".
> This patch prepends "/proc" prefix if the pathname was obtained inside
> binary_sysctl() so that TOMOYO checks a pathname which starts with "/proc/sys".
>
> Signed-off-by: Tetsuo Handa <[email protected]>
> ---
> security/tomoyo/realpath.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> --- sysctl-2.6.orig/security/tomoyo/realpath.c
> +++ sysctl-2.6/security/tomoyo/realpath.c
> @@ -108,6 +108,14 @@ int tomoyo_realpath_from_path2(struct pa
> spin_unlock(&dcache_lock);
> path_put(&root);
> path_put(&ns_root);
> + /* Prepend "/proc" prefix if binary_sysctl(). */
> + if (!IS_ERR(sp) && current->in_sysctl) {
> + sp -= 5;
> + if (sp >= newname)
> + memcpy(sp, "/proc", 5);
> + else
> + sp = ERR_PTR(-ENOMEM);
> + }
> }
> if (IS_ERR(sp))
> error = PTR_ERR(sp);

Instead of current->in_sysctl we can just look at the path and see if
it is the root of the mount chain and if the fs is proc.

Something like:

diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 5f2e332..0b55faa 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -108,6 +108,15 @@ int tomoyo_realpath_from_path2(struct path *path, char *newname,
spin_unlock(&dcache_lock);
path_put(&root);
path_put(&ns_root);
+ /* Prepend "/proc" prefix if using internal proc vfs mount. */
+ if (!IS_ERR(sp) && (path->mnt->mnt_parent == path->mnt) &&
+ (strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0)) {
+ sp -= 5;
+ if (sp >= newname)
+ memcpy(sp, "/proc", 5);
+ else
+ sp = ERR_PTR(-ENOMEM);
+ }
}
if (IS_ERR(sp))
error = PTR_ERR(sp);

Eric

2009-11-19 22:17:54

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Hello.

Eric W. Biederman wrote:
> > Indeed. TOMOYO and AppArmor need a hint for prepending "/proc" prefix.
> > A simple implementation which adds one bit to task_struct is shown below.
> > In this way, not only the file permission checks inside dentry_open()
> > but also the directory permission checks inside vfs_path_lookup() can be
> > prepended "/proc" prefix. AppArmor might want to prepend "/proc" inside
> > vfs_path_lookup().
>
> There don't appear to be any security hooks in vfs_path_lookup().
>
OK. Then, AppArmor won't be confused.

> Instead of current->in_sysctl we can just look at the path and see if
> it is the root of the mount chain and if the fs is proc.
>
> Something like:
>
> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
> index 5f2e332..0b55faa 100644
> --- a/security/tomoyo/realpath.c
> +++ b/security/tomoyo/realpath.c
> @@ -108,6 +108,15 @@ int tomoyo_realpath_from_path2(struct path *path, char *newname,
> spin_unlock(&dcache_lock);
> path_put(&root);
> path_put(&ns_root);
> + /* Prepend "/proc" prefix if using internal proc vfs mount. */
> + if (!IS_ERR(sp) && (path->mnt->mnt_parent == path->mnt) &&
> + (strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0)) {
> + sp -= 5;
> + if (sp >= newname)
> + memcpy(sp, "/proc", 5);
> + else
> + sp = ERR_PTR(-ENOMEM);
> + }
> }
> if (IS_ERR(sp))
> error = PTR_ERR(sp);

Above patch works. Please proceed. Thank you.

Acked-by: Tetsuo Handa <[email protected]>

Why not to use path->mnt->mnt_sb->s_magic == PROC_SUPER_MAGIC rather than
strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0 ?

2009-11-19 22:22:52

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Tetsuo Handa <[email protected]> writes:

> Hello.
>
> Eric W. Biederman wrote:
>> > Indeed. TOMOYO and AppArmor need a hint for prepending "/proc" prefix.
>> > A simple implementation which adds one bit to task_struct is shown below.
>> > In this way, not only the file permission checks inside dentry_open()
>> > but also the directory permission checks inside vfs_path_lookup() can be
>> > prepended "/proc" prefix. AppArmor might want to prepend "/proc" inside
>> > vfs_path_lookup().
>>
>> There don't appear to be any security hooks in vfs_path_lookup().
>>
> OK. Then, AppArmor won't be confused.
>
>> Instead of current->in_sysctl we can just look at the path and see if
>> it is the root of the mount chain and if the fs is proc.
>>
>> Something like:
>>
>> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
>> index 5f2e332..0b55faa 100644
>> --- a/security/tomoyo/realpath.c
>> +++ b/security/tomoyo/realpath.c
>> @@ -108,6 +108,15 @@ int tomoyo_realpath_from_path2(struct path *path, char *newname,
>> spin_unlock(&dcache_lock);
>> path_put(&root);
>> path_put(&ns_root);
>> + /* Prepend "/proc" prefix if using internal proc vfs mount. */
>> + if (!IS_ERR(sp) && (path->mnt->mnt_parent == path->mnt) &&
>> + (strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0)) {
>> + sp -= 5;
>> + if (sp >= newname)
>> + memcpy(sp, "/proc", 5);
>> + else
>> + sp = ERR_PTR(-ENOMEM);
>> + }
>> }
>> if (IS_ERR(sp))
>> error = PTR_ERR(sp);
>
> Above patch works. Please proceed. Thank you.
>
> Acked-by: Tetsuo Handa <[email protected]>
>
> Why not to use path->mnt->mnt_sb->s_magic == PROC_SUPER_MAGIC rather than
> strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0 ?

Brain short circuit.

Eric

2009-11-19 22:35:10

by John Johansen

[permalink] [raw]

Subject: Re: [PATCH 00/23] Removal of binary sysctl support

Eric W. Biederman wrote:
> Tetsuo Handa <[email protected]> writes:
>
>> Hello.
>>
>> Eric W. Biederman wrote:
>>>> Indeed. TOMOYO and AppArmor need a hint for prepending "/proc" prefix.
>>>> A simple implementation which adds one bit to task_struct is shown below.
>>>> In this way, not only the file permission checks inside dentry_open()
>>>> but also the directory permission checks inside vfs_path_lookup() can be
>>>> prepended "/proc" prefix. AppArmor might want to prepend "/proc" inside
>>>> vfs_path_lookup().
>>> There don't appear to be any security hooks in vfs_path_lookup().
>>>
>> OK. Then, AppArmor won't be confused.
>>
>>> Instead of current->in_sysctl we can just look at the path and see if
>>> it is the root of the mount chain and if the fs is proc.
>>>
>>> Something like:
>>>
>>> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
>>> index 5f2e332..0b55faa 100644
>>> --- a/security/tomoyo/realpath.c
>>> +++ b/security/tomoyo/realpath.c
>>> @@ -108,6 +108,15 @@ int tomoyo_realpath_from_path2(struct path *path, char *newname,
>>> spin_unlock(&dcache_lock);
>>> path_put(&root);
>>> path_put(&ns_root);
>>> + /* Prepend "/proc" prefix if using internal proc vfs mount. */
>>> + if (!IS_ERR(sp) && (path->mnt->mnt_parent == path->mnt) &&
>>> + (strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0)) {
>>> + sp -= 5;
>>> + if (sp >= newname)
>>> + memcpy(sp, "/proc", 5);
>>> + else
>>> + sp = ERR_PTR(-ENOMEM);
>>> + }
>>> }
>>> if (IS_ERR(sp))
>>> error = PTR_ERR(sp);
>> Above patch works. Please proceed. Thank you.
>>
>> Acked-by: Tetsuo Handa <[email protected]>
>>
>> Why not to use path->mnt->mnt_sb->s_magic == PROC_SUPER_MAGIC rather than
>> strcmp(path->mnt->mnt_sb->s_type->name, "proc") == 0 ?
>
> Brain short circuit.
>
The patch look good to me too.

Acked-by: John Johansen <[email protected]>