netdev - Re: [PATCH RESEND 1/3] netfilter: xtables: inclusion of xt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1285099185.10579.17.camel@powerslave>
Date:	Tue, 21 Sep 2010 22:59:45 +0300
From:	Luciano Coelho <luciano.coelho@...ia.com>
To:	ext Patrick McHardy <kaber@...sh.net>
Cc:	"netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"jengelh@...ozas.de" <jengelh@...ozas.de>,
	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"Ylalehto Janne (Nokia-MS/Tampere)" <janne.ylalehto@...ia.com>
Subject: Re: [PATCH RESEND 1/3] netfilter: xtables: inclusion of
 xt_condition

On Wed, 2010-09-15 at 22:39 +0200, ext Patrick McHardy wrote:
> Am 17.08.2010 10:36, schrieb Luciano Coelho:
> > diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c
> > new file mode 100644
> > index 0000000..a78d832
> > --- /dev/null
> > +++ b/net/netfilter/xt_condition.c
> > @@ -0,0 +1,265 @@
> > +/*
> > + *	"condition" match extension for Xtables
> > + *
> > + *	Description: This module allows firewall rules to match using
> > + *	condition variables available through procfs.
> > + *
> > + *	Authors:
> > + *	Stephane Ouellette <ouellettes [at] videotron ca>, 2002-10-22
> > + *	Massimiliano Hofer <max [at] nucleus it>, 2006-05-15
> > + *
> > + *	This program is free software; you can redistribute it and/or modify it
> > + *	under the terms of the GNU General Public License; either version 2
> > + *	or 3 of the License, as published by the Free Software Foundation.
> > + */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +#include <linux/version.h>
> > +#include <linux/netfilter/x_tables.h>
> > +#include <linux/netfilter/xt_condition.h>
> > +#include <net/netns/generic.h>
> > +#include <asm/uaccess.h>
> > +
> > +/* Defaults, these can be overridden on the module command-line. */
> > +static unsigned int condition_list_perms = S_IRUGO | S_IWUSR;
> > +static unsigned int condition_uid_perms = 0;
> > +static unsigned int condition_gid_perms = 0;
> 
> I'm not sure whether we already discussed this, but this isn't
> useful if namespaces are used since the IDs aren't global.

As Jan suggested, I think it would be better to include it like this,
since other modules already do similar things, and then I'll later send
a new patch with support for multiple namespaces.


> > +
> > +MODULE_AUTHOR("Stephane Ouellette <ouellettes@...eotron.ca>");
> > +MODULE_AUTHOR("Massimiliano Hofer <max@...leus.it>");
> > +MODULE_AUTHOR("Jan Engelhardt <jengelh@...ozas.de>");
> > +MODULE_DESCRIPTION("Allows rules to match against condition variables");
> > +MODULE_LICENSE("GPL");
> > +module_param(condition_list_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_list_perms, "default permissions on /proc/net/nf_condition/* files");
> > +module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files");
> > +module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR);
> > +MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files");
> > +MODULE_ALIAS("ipt_condition");
> > +MODULE_ALIAS("ip6t_condition");
> > +
> 
> > +/* proc_lock is a user context only semaphore used for write access */
> > +/*           to the conditions' list.                               */
> > +static DEFINE_MUTEX(proc_lock);
> 
> Why is this called proc_lock, as the comment states it protects
> the condition variable list? The comment is also misleading since
> this is a mutex and not a semaphore. I'd suggest list_mutex or
> condition_mutex.

I'll fix this.


> > +static int condition_proc_read(char __user *buffer, char **start, off_t offset,
> > +			       int length, int *eof, void *data)
> > +{
> > +	const struct condition_variable *var = data;
> > +
> > +	buffer[0] = var->enabled ? '1' : '0';
> > +	buffer[1] = '\n';
> 
> This is a user buffer, you need copy_to_user(). Also please run
> sparse against your code to check for similar errors.

Oooff, I actually also noticed this when integrating into our internal
tree.  I'll fix this and run sparse properly before resending.


-- 
Cheers,
Luca.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html