Re: [TOMOYO #10 (linux-next) 3/8] LSM adapter functions.

[KAMEZAWA Hiroyuki]

Hi,

some nitpicks.

On Thu, 09 Oct 2008 13:28:17 +0900
Kentaro Takeda <takedakn@xxxxxxxxxxxxx> wrote:

> Signed-off-by: Kentaro Takeda <takedakn@xxxxxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Toshiharu Harada <haradats@xxxxxxxxxxxxx>
> ---
>  security/tomoyo/tomoyo.c |  319 +++++++++++++++++++++++++++++++++++++++++++++++
>  security/tomoyo/tomoyo.h |  106 +++++++++++++++
>  2 files changed, 425 insertions(+)
> 
> --- /dev/null
> +++ linux-next/security/tomoyo/tomoyo.c
> @@ -0,0 +1,319 @@
> +/*
> + * security/tomoyo/tomoyo.c
> + *
> + * LSM hooks for TOMOYO Linux.
> + *
> + * Copyright (C) 2005-2008  NTT DATA CORPORATION
> + *
> + * Version: 2.2.0-pre   2008/10/01
> + *
> + */

What is this Version for ?
Is this exported to userland via some interface ?

> +
> +#include <linux/security.h>
> +#include "common.h"
> +#include "tomoyo.h"
> +#include "realpath.h"
> +#include <linux/audit.h>
> +#include <linux/device_cgroup.h>
> +
> +static int tmy_cred_prepare(struct cred *new, const struct cred *old, gfp_t gfp)
> +{
> +	new->security = old->security;
> +	return 0;
> +}
> +
> +static int tmy_bprm_set_creds(struct linux_binprm *bprm)
> +{
> +	if (bprm->cred_prepared)
> +		return 0;
> +	if (!sbin_init_started)
> +		tmy_load_policy(bprm->filename);
> +	bprm->cred->security = NULL;
> +	return 0;
> +}
> +
> +static int tmy_bprm_check_security(struct linux_binprm *bprm)
> +{
> +	struct domain_info *domain = bprm->cred->security;
> +	/*
> +	 * Execute permission is checked against pathname passed to do_execve()
> +	 * using current domain.
> +	 */
> +	if (!domain) {
> +		struct domain_info *next_domain = NULL;
> +		int retval = tmy_find_next_domain(bprm, &next_domain);
> +		if (!retval)
> +			bprm->cred->security = next_domain;
> +		return retval;
> +	}
> +	/*
> +	 * Read permission is checked against interpreters using next domain.
> +	 */
> +	return tmy_check_open_permission(domain, bprm->file->f_path.dentry,
> +					 bprm->file->f_path.mnt, 1);
> +}
> +
> +static int tmy_sysctl(struct ctl_table *table, int op)
> +{
> +	int error;
> +	char *name;
> +	if ((op & 6) == 0)
> +		return 0;

It seems that you need a blanc line between variable declaration and
start of code, like this.

==
  char *name;
                                 << 
  if ((op & 6) == 0)
		return 0;
==

BTW, is this "6" need to be "6" rather than some readable macro ?

<snip>
> +static int tmy_dentry_open(struct file *f, const struct cred *cred)
> +{
> +	int flags = f->f_flags;
> +	if ((flags + 1) & O_ACCMODE)
> +		flags++;
> +	flags |= f->f_flags & (O_APPEND | O_TRUNC);
> +	/* Don't check read permission here if called from do_execve(). */
> +	if (current->in_execve)
> +		return 0;
> +	return tmy_check_open_permission(tmy_domain(), f->f_path.dentry,
> +					 f->f_path.mnt, flags);
> +}
> +

This tmy_check_open_permission() is defiend in 7/8.
Because people uses "bisect" in these days, I think it's better to avoid
this kind of reversed dependency of patch stack.


Thanks,
-Kame


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/