* Re: procfs does not return error code when file name is already in use
2012-10-26 9:57 procfs does not return error code when file name is already in use Дмитрий Леонтьев
@ 2012-10-26 10:06 ` Jiri Kosina
2012-10-26 12:05 ` Thadeu Lima de Souza Cascardo
2012-10-26 11:03 ` Al Viro
2012-10-26 12:22 ` Thadeu Lima de Souza Cascardo
2 siblings, 1 reply; 5+ messages in thread
From: Jiri Kosina @ 2012-10-26 10:06 UTC (permalink / raw)
To: Дмитрий Леонтьев
Cc: linux-kernel, dleontie, Andrew Morton, Al Viro
On Fri, 26 Oct 2012, Дмитрий Леонтьев wrote:
> I'm working a kernel module, and one of my tasks is to disallow a
> process to open my character device(/dev/xxx) more than once. I tried
> to solve this problem by creating /proc/apu directory, and creating a
> /proc/xxx/{pid} file for when process opens /dev/xxx. This file will
> act as per-process mutex, and provide a way to control resource usage.
Now that's a complex solution indeed! :)
Why not just handle that by looking at struct inode* passed to your open()
callback and checking whether you have that open already, for example?
> Then, I investigated,if procfs acts as I expect from it, and found
> that procfs checks file names for uniqueness, but does not return an
> error when I add two files with same name. Instead, it posts silly
> messages to kernel log, and adds new entry as if nothing happened.
> This is a vulnerability:
>
> Let we have 2 modules A and B trying to create file /proc/foo on load,
> and removing it on unload. Load A, Load B, unload B. at step 3 B
> removed /proc/foo owned by A, and /proc/foo created by B remains
> accessible from userspace.
>
> My patch fixes this behaviour: proc_register will return -EEXIST when
> file name is in use, and refuse to add a duplicate.
I wouldn't call it a vulnerability, but a bug, probably yes. Adding Andrew
and Al to CC.
>
> ----start of patch for fs/proc/generic.c---------------------------------------
> --- generic.c.orig 2012-10-25 22:20:05.196606263 +0400
> +++ generic.c 2012-10-25 22:13:49.556955392 +0400
Please always generate the patches against kernel tree in a form so that
they could be applied using patch -p1.
> @@ -554,45 +554,51 @@ static const struct inode_operations pro
>
> static int proc_register(struct proc_dir_entry * dir, struct
> proc_dir_entry * dp)
> {
> + int errorcode=-EAGAIN;
> unsigned int i;
> struct proc_dir_entry *tmp;
>
> i = get_inode_number();
> - if (i == 0)
> - return -EAGAIN;
> - dp->low_ino = i;
> -
> - if (S_ISDIR(dp->mode)) {
> - if (dp->proc_iops == NULL) {
> - dp->proc_fops = &proc_dir_operations;
> - dp->proc_iops = &proc_dir_inode_operations;
> + if (i != 0)
> + {
> + errorcode=0;
> +
> + dp->low_ino = i;
> +
> + if (S_ISDIR(dp->mode)) {
> + if (dp->proc_iops == NULL) {
> + dp->proc_fops = &proc_dir_operations;
> + dp->proc_iops = &proc_dir_inode_operations;
> + }
> + dir->nlink++;
> + } else if (S_ISLNK(dp->mode)) {
> + if (dp->proc_iops == NULL)
> + dp->proc_iops = &proc_link_inode_operations;
> + } else if (S_ISREG(dp->mode)) {
> + if (dp->proc_fops == NULL)
> + dp->proc_fops = &proc_file_operations;
> + if (dp->proc_iops == NULL)
> + dp->proc_iops = &proc_file_inode_operations;
> }
> - dir->nlink++;
> - } else if (S_ISLNK(dp->mode)) {
> - if (dp->proc_iops == NULL)
> - dp->proc_iops = &proc_link_inode_operations;
> - } else if (S_ISREG(dp->mode)) {
> - if (dp->proc_fops == NULL)
> - dp->proc_fops = &proc_file_operations;
> - if (dp->proc_iops == NULL)
> - dp->proc_iops = &proc_file_inode_operations;
> - }
>
> - spin_lock(&proc_subdir_lock);
> + spin_lock(&proc_subdir_lock);
>
> - for (tmp = dir->subdir; tmp; tmp = tmp->next)
> - if (strcmp(tmp->name, dp->name) == 0) {
> - WARN(1, KERN_WARNING "proc_dir_entry '%s/%s' already registered\n",
> - dir->name, dp->name);
> - break;
> + for (tmp = dir->subdir; tmp; tmp = tmp->next)
> + {
> + if (strcmp(tmp->name, dp->name) == 0) {
> + errorcode=-EEXIST;
> + break;
> + }
> }
> -
> - dp->next = dir->subdir;
> - dp->parent = dir;
> - dir->subdir = dp;
> - spin_unlock(&proc_subdir_lock);
> -
> - return 0;
> + if(!errorcode)
> + {
> + dp->next = dir->subdir;
> + dp->parent = dir;
> + dir->subdir = dp;
> + }
> + spin_unlock(&proc_subdir_lock);
> + }
> + return errorcode;
> }
>
> static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
> ----end of patch---------------------------------------
>
> Regards
> Dmitry
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: procfs does not return error code when file name is already in use
2012-10-26 10:06 ` Jiri Kosina
@ 2012-10-26 12:05 ` Thadeu Lima de Souza Cascardo
0 siblings, 0 replies; 5+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2012-10-26 12:05 UTC (permalink / raw)
To: Jiri Kosina
Cc: Дмитрий Леонтьев,
linux-kernel, dleontie, Andrew Morton, Al Viro
On Fri, Oct 26, 2012 at 12:06:08PM +0200, Jiri Kosina wrote:
> On Fri, 26 Oct 2012, Дмитрий Леонтьев wrote:
>
> > I'm working a kernel module, and one of my tasks is to disallow a
> > process to open my character device(/dev/xxx) more than once. I tried
> > to solve this problem by creating /proc/apu directory, and creating a
> > /proc/xxx/{pid} file for when process opens /dev/xxx. This file will
> > act as per-process mutex, and provide a way to control resource usage.
>
> Now that's a complex solution indeed! :)
>
> Why not just handle that by looking at struct inode* passed to your open()
> callback and checking whether you have that open already, for example?
>
> > Then, I investigated,if procfs acts as I expect from it, and found
> > that procfs checks file names for uniqueness, but does not return an
> > error when I add two files with same name. Instead, it posts silly
> > messages to kernel log, and adds new entry as if nothing happened.
> > This is a vulnerability:
> >
> > Let we have 2 modules A and B trying to create file /proc/foo on load,
> > and removing it on unload. Load A, Load B, unload B. at step 3 B
> > removed /proc/foo owned by A, and /proc/foo created by B remains
> > accessible from userspace.
> >
> > My patch fixes this behaviour: proc_register will return -EEXIST when
> > file name is in use, and refuse to add a duplicate.
>
> I wouldn't call it a vulnerability, but a bug, probably yes. Adding Andrew
> and Al to CC.
>
The real bug is when some code happens to create the same procfile
again. Perhaps because the module removal didn't remove the file. This
WARN is a good debug tool for those real bugs.
I can't think of a real usecase where it would be legitimate to try to
create a procfile which could possibly have been created before.
The only case is the one described here, where two different and
unrelated codes that could be simultaneously loaded try to create the
same file. In this case, I believe either only one of them should be
allowed to be loaded (and perhaps failing the creation of the file might
accomplish that) or they should be using different files (perhaps on a
common directory for that class of files).
Regards.
Cascardo.
> >
> > ----start of patch for fs/proc/generic.c---------------------------------------
> > --- generic.c.orig 2012-10-25 22:20:05.196606263 +0400
> > +++ generic.c 2012-10-25 22:13:49.556955392 +0400
>
> Please always generate the patches against kernel tree in a form so that
> they could be applied using patch -p1.
>
> > @@ -554,45 +554,51 @@ static const struct inode_operations pro
> >
> > static int proc_register(struct proc_dir_entry * dir, struct
> > proc_dir_entry * dp)
> > {
> > + int errorcode=-EAGAIN;
> > unsigned int i;
> > struct proc_dir_entry *tmp;
> >
> > i = get_inode_number();
> > - if (i == 0)
> > - return -EAGAIN;
> > - dp->low_ino = i;
> > -
> > - if (S_ISDIR(dp->mode)) {
> > - if (dp->proc_iops == NULL) {
> > - dp->proc_fops = &proc_dir_operations;
> > - dp->proc_iops = &proc_dir_inode_operations;
> > + if (i != 0)
> > + {
> > + errorcode=0;
> > +
> > + dp->low_ino = i;
> > +
> > + if (S_ISDIR(dp->mode)) {
> > + if (dp->proc_iops == NULL) {
> > + dp->proc_fops = &proc_dir_operations;
> > + dp->proc_iops = &proc_dir_inode_operations;
> > + }
> > + dir->nlink++;
> > + } else if (S_ISLNK(dp->mode)) {
> > + if (dp->proc_iops == NULL)
> > + dp->proc_iops = &proc_link_inode_operations;
> > + } else if (S_ISREG(dp->mode)) {
> > + if (dp->proc_fops == NULL)
> > + dp->proc_fops = &proc_file_operations;
> > + if (dp->proc_iops == NULL)
> > + dp->proc_iops = &proc_file_inode_operations;
> > }
> > - dir->nlink++;
> > - } else if (S_ISLNK(dp->mode)) {
> > - if (dp->proc_iops == NULL)
> > - dp->proc_iops = &proc_link_inode_operations;
> > - } else if (S_ISREG(dp->mode)) {
> > - if (dp->proc_fops == NULL)
> > - dp->proc_fops = &proc_file_operations;
> > - if (dp->proc_iops == NULL)
> > - dp->proc_iops = &proc_file_inode_operations;
> > - }
> >
> > - spin_lock(&proc_subdir_lock);
> > + spin_lock(&proc_subdir_lock);
> >
> > - for (tmp = dir->subdir; tmp; tmp = tmp->next)
> > - if (strcmp(tmp->name, dp->name) == 0) {
> > - WARN(1, KERN_WARNING "proc_dir_entry '%s/%s' already registered\n",
> > - dir->name, dp->name);
> > - break;
> > + for (tmp = dir->subdir; tmp; tmp = tmp->next)
> > + {
> > + if (strcmp(tmp->name, dp->name) == 0) {
> > + errorcode=-EEXIST;
> > + break;
> > + }
> > }
> > -
> > - dp->next = dir->subdir;
> > - dp->parent = dir;
> > - dir->subdir = dp;
> > - spin_unlock(&proc_subdir_lock);
> > -
> > - return 0;
> > + if(!errorcode)
> > + {
> > + dp->next = dir->subdir;
> > + dp->parent = dir;
> > + dir->subdir = dp;
> > + }
> > + spin_unlock(&proc_subdir_lock);
> > + }
> > + return errorcode;
> > }
> >
> > static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
> > ----end of patch---------------------------------------
> >
> > Regards
> > Dmitry
> >
>
> --
> Jiri Kosina
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: procfs does not return error code when file name is already in use
2012-10-26 9:57 procfs does not return error code when file name is already in use Дмитрий Леонтьев
2012-10-26 10:06 ` Jiri Kosina
@ 2012-10-26 11:03 ` Al Viro
2012-10-26 12:22 ` Thadeu Lima de Souza Cascardo
2 siblings, 0 replies; 5+ messages in thread
From: Al Viro @ 2012-10-26 11:03 UTC (permalink / raw)
To: ??????? ????????; +Cc: linux-kernel, trivial, dleontie
On Fri, Oct 26, 2012 at 01:57:43PM +0400, ??????? ???????? wrote:
> I'm working a kernel module, and one of my tasks is to disallow a
> process to open my character device(/dev/xxx) more than once. I tried
> to solve this problem by creating /proc/apu directory, and creating a
> /proc/xxx/{pid} file for when process opens /dev/xxx. This file will
> act as per-process mutex, and provide a way to control resource usage.
Whoever had set that "task" upon you deserves to be painfully educated
until they realize that thus stated the limitation is absolutely worthless -
file does not have to be opened by the same process that uses it. IOW,
limiting opens to one per process does not provide any kind of resource
control - it's trivially bypassed by creating an AF_UNIX socketpair,
then forking children in a loop, each doing open + send SCM_RIGHTS
datagram to parent over that socket with opened fd in it + exit. And
receiving those datagrams until you get as many opened descriptors for
your device as you wanted.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: procfs does not return error code when file name is already in use
2012-10-26 9:57 procfs does not return error code when file name is already in use Дмитрий Леонтьев
2012-10-26 10:06 ` Jiri Kosina
2012-10-26 11:03 ` Al Viro
@ 2012-10-26 12:22 ` Thadeu Lima de Souza Cascardo
2 siblings, 0 replies; 5+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2012-10-26 12:22 UTC (permalink / raw)
To: Дмитрий Леонтьев
Cc: linux-kernel, Jiri Kosina, dleontie, Andrew Morton, Al Viro
On Fri, Oct 26, 2012 at 01:57:43PM +0400, Дмитрий Леонтьев wrote:
> I'm working a kernel module, and one of my tasks is to disallow a
> process to open my character device(/dev/xxx) more than once. I tried
> to solve this problem by creating /proc/apu directory, and creating a
> /proc/xxx/{pid} file for when process opens /dev/xxx. This file will
> act as per-process mutex, and provide a way to control resource usage.
>
> Then, I investigated,if procfs acts as I expect from it, and found
> that procfs checks file names for uniqueness, but does not return an
> error when I add two files with same name. Instead, it posts silly
> messages to kernel log, and adds new entry as if nothing happened.
> This is a vulnerability:
>
> Let we have 2 modules A and B trying to create file /proc/foo on load,
> and removing it on unload. Load A, Load B, unload B. at step 3 B
> removed /proc/foo owned by A, and /proc/foo created by B remains
> accessible from userspace.
>
> My patch fixes this behaviour: proc_register will return -EEXIST when
> file name is in use, and refuse to add a duplicate.
>
> ----start of patch for fs/proc/generic.c---------------------------------------
> --- generic.c.orig 2012-10-25 22:20:05.196606263 +0400
> +++ generic.c 2012-10-25 22:13:49.556955392 +0400
> @@ -554,45 +554,51 @@ static const struct inode_operations pro
>
You just push a lot of code a level deep, without any need, and don't
handle your error path properly.
> static int proc_register(struct proc_dir_entry * dir, struct
> proc_dir_entry * dp)
> {
> + int errorcode=-EAGAIN;
You have lots of coding style problems like this. Please use spaces
before and after =, like this:
int errorcode = -EAGAIN;
> unsigned int i;
> struct proc_dir_entry *tmp;
>
> i = get_inode_number();
> - if (i == 0)
> - return -EAGAIN;
You must use release_inode_number if you go into the error path. If you
want to distinguish between this error path and the -EEXIST one, you'd
better keep this as it is. Just return the error here and don't do the
extra indentation.
> - dp->low_ino = i;
> -
> - if (S_ISDIR(dp->mode)) {
> - if (dp->proc_iops == NULL) {
> - dp->proc_fops = &proc_dir_operations;
> - dp->proc_iops = &proc_dir_inode_operations;
> + if (i != 0)
> + {
> + errorcode=0;
> +
> + dp->low_ino = i;
> +
> + if (S_ISDIR(dp->mode)) {
> + if (dp->proc_iops == NULL) {
> + dp->proc_fops = &proc_dir_operations;
> + dp->proc_iops = &proc_dir_inode_operations;
> + }
> + dir->nlink++;
Same here. You must undo the extra link, if you go through an error
path.
> + } else if (S_ISLNK(dp->mode)) {
> + if (dp->proc_iops == NULL)
> + dp->proc_iops = &proc_link_inode_operations;
> + } else if (S_ISREG(dp->mode)) {
> + if (dp->proc_fops == NULL)
> + dp->proc_fops = &proc_file_operations;
> + if (dp->proc_iops == NULL)
> + dp->proc_iops = &proc_file_inode_operations;
> }
> - dir->nlink++;
> - } else if (S_ISLNK(dp->mode)) {
> - if (dp->proc_iops == NULL)
> - dp->proc_iops = &proc_link_inode_operations;
> - } else if (S_ISREG(dp->mode)) {
> - if (dp->proc_fops == NULL)
> - dp->proc_fops = &proc_file_operations;
> - if (dp->proc_iops == NULL)
> - dp->proc_iops = &proc_file_inode_operations;
> - }
>
> - spin_lock(&proc_subdir_lock);
> + spin_lock(&proc_subdir_lock);
>
> - for (tmp = dir->subdir; tmp; tmp = tmp->next)
> - if (strcmp(tmp->name, dp->name) == 0) {
> - WARN(1, KERN_WARNING "proc_dir_entry '%s/%s' already registered\n",
> - dir->name, dp->name);
> - break;
Please, keep the warning. In my experience, it points out to real bugs
in new code.
> + for (tmp = dir->subdir; tmp; tmp = tmp->next)
> + {
> + if (strcmp(tmp->name, dp->name) == 0) {
> + errorcode=-EEXIST;
> + break;
> + }
> }
> -
> - dp->next = dir->subdir;
> - dp->parent = dir;
> - dir->subdir = dp;
> - spin_unlock(&proc_subdir_lock);
> -
> - return 0;
> + if(!errorcode)
> + {
> + dp->next = dir->subdir;
> + dp->parent = dir;
> + dir->subdir = dp;
> + }
> + spin_unlock(&proc_subdir_lock);
> + }
> + return errorcode;
> }
>
> static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
> ----end of patch---------------------------------------
>
> Regards
> Dmitry
> --
Regards.
Cascardo.
^ permalink raw reply [flat|nested] 5+ messages in thread