Driver-Core: devtmpfs - reset inode permissions before unlinking

Driver-Core: devtmpfs - reset inode permissions before unlinking

[Kay Sievers]

From: Kay Sievers <kay.sievers@vrfy.org>
Subject: Driver-Core: devtmpfs - reset inode permissions before unlinking

Before unlinking the inode, reset the current permissions of possible
references like hardlinks, so granted permissions can not be retained
across the device lifetime by creating hardlinks, in the unusual case
that there is a user-writable directory on the same filesystem.

Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/base/devtmpfs.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -301,6 +301,19 @@ int devtmpfs_delete_node(struct device *
 		if (dentry->d_inode) {
 			err = vfs_getattr(nd.path.mnt, dentry, &stat);
 			if (!err && dev_mynode(dev, dentry->d_inode, &stat)) {
+				struct iattr newattrs;
+				/*
+				 * before unlinking this node, reset permissions
+				 * of possible references like hardlinks
+				 */
+				newattrs.ia_uid = 0;
+				newattrs.ia_gid = 0;
+				newattrs.ia_mode = stat.mode & ~0777;
+				newattrs.ia_valid =
+					ATTR_UID|ATTR_GID|ATTR_MODE;
+				mutex_lock(&dentry->d_inode->i_mutex);
+				notify_change(dentry, &newattrs);
+				mutex_unlock(&dentry->d_inode->i_mutex);
 				err = vfs_unlink(nd.path.dentry->d_inode,
 						 dentry);
 				if (!err || err == -ENOENT)

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Greg KH]

On Wed, Jan 13, 2010 at 02:16:36PM +0100, Kay Sievers wrote:
> From: Kay Sievers <kay.sievers@vrfy.org>
> Subject: Driver-Core: devtmpfs - reset inode permissions before unlinking
> 
> Before unlinking the inode, reset the current permissions of possible
> references like hardlinks, so granted permissions can not be retained
> across the device lifetime by creating hardlinks, in the unusual case
> that there is a user-writable directory on the same filesystem.

Is this something that we need to worry about for existing users
(2.6.32 and .33), or can it wait until 2.6.34?

thanks,

greg k-h

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Kay Sievers]

On Thu, Jan 14, 2010 at 04:49, Greg KH <greg@kroah.com> wrote:
> On Wed, Jan 13, 2010 at 02:16:36PM +0100, Kay Sievers wrote:
>> From: Kay Sievers <kay.sievers@vrfy.org>
>> Subject: Driver-Core: devtmpfs - reset inode permissions before unlinking
>>
>> Before unlinking the inode, reset the current permissions of possible
>> references like hardlinks, so granted permissions can not be retained
>> across the device lifetime by creating hardlinks, in the unusual case
>> that there is a user-writable directory on the same filesystem.
>
> Is this something that we need to worry about for existing users
> (2.6.32 and .33), or can it wait until 2.6.34?

Should be fine for .34 only.

Thanks,
Kay

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Henrique de Moraes Holschuh]

On Wed, 13 Jan 2010, Kay Sievers wrote:
> across the device lifetime by creating hardlinks, in the unusual case
> that there is a user-writable directory on the same filesystem.

Does a tmpfs mounted in /dev/shm count as "user-writable directory on the
same filesystem" ?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Kay Sievers]

On Thu, Jan 14, 2010 at 21:53, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Wed, 13 Jan 2010, Kay Sievers wrote:
>> across the device lifetime by creating hardlinks, in the unusual case
>> that there is a user-writable directory on the same filesystem.
>
> Does a tmpfs mounted in /dev/shm count as "user-writable directory on the
> same filesystem" ?

Not if it's a separate tmpfs mount, which is recommended. Only if it's
just a plain directory on the /dev filesystem.

Udev does the same and resets the permissions of the inode before deleting it.

Kay

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Henrique de Moraes Holschuh]

On Thu, 14 Jan 2010, Henrique de Moraes Holschuh wrote:
> On Wed, 13 Jan 2010, Kay Sievers wrote:
> > across the device lifetime by creating hardlinks, in the unusual case
> > that there is a user-writable directory on the same filesystem.
> 
> Does a tmpfs mounted in /dev/shm count as "user-writable directory on the
> same filesystem" ?

Never mind... stupid question really.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Henrique de Moraes Holschuh]

On Thu, 14 Jan 2010, Kay Sievers wrote:
> On Thu, Jan 14, 2010 at 21:53, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
> > On Wed, 13 Jan 2010, Kay Sievers wrote:
> >> across the device lifetime by creating hardlinks, in the unusual case
> >> that there is a user-writable directory on the same filesystem.
> >
> > Does a tmpfs mounted in /dev/shm count as "user-writable directory on the
> > same filesystem" ?
> 
> Not if it's a separate tmpfs mount, which is recommended. Only if it's
> just a plain directory on the /dev filesystem.

Yeah, I noticed the abusurdity of my question when I re-read it, thanks for
being kind in the reply.

That said, this does fix a possible security problem when a misconfigured
system is used, and the fix looks rather simple...  Can it go to -stable
eventually, even if it is months in the future, after it gets some testing
in .34 ?   Minor problems are still problems...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Kay Sievers]

On Thu, Jan 14, 2010 at 22:07, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Thu, 14 Jan 2010, Kay Sievers wrote:
>> On Thu, Jan 14, 2010 at 21:53, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>> > On Wed, 13 Jan 2010, Kay Sievers wrote:
>> >> across the device lifetime by creating hardlinks, in the unusual case
>> >> that there is a user-writable directory on the same filesystem.
>> >
>> > Does a tmpfs mounted in /dev/shm count as "user-writable directory on the
>> > same filesystem" ?
>>
>> Not if it's a separate tmpfs mount, which is recommended. Only if it's
>> just a plain directory on the /dev filesystem.
>
> Yeah, I noticed the abusurdity of my question when I re-read it, thanks for
> being kind in the reply.
>
> That said, this does fix a possible security problem when a misconfigured
> system is used, and the fix looks rather simple...  Can it go to -stable
> eventually, even if it is months in the future, after it gets some testing
> in .34 ?   Minor problems are still problems...

Sure, we could do that. There is some stuff in the current .33 kernel,
which could go into .32-stable too, if that's useful.

Kay

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Henrique de Moraes Holschuh]

On Thu, 14 Jan 2010, Kay Sievers wrote:
> > That said, this does fix a possible security problem when a misconfigured
> > system is used, and the fix looks rather simple...  Can it go to -stable
> > eventually, even if it is months in the future, after it gets some testing
> > in .34 ?   Minor problems are still problems...
> 
> Sure, we could do that. There is some stuff in the current .33 kernel,
> which could go into .32-stable too, if that's useful.

I think it probably would be useful.  I understand .32 is going to stay with
us for a long time, so it should get any fixes that have withstood the test
of time.

It is very annoying to have subtly different kernel behaviour (from
mainline) in a long-term stable series...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Greg KH]

On Sat, Jan 16, 2010 at 12:26:41AM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 14 Jan 2010, Kay Sievers wrote:
> > > That said, this does fix a possible security problem when a misconfigured
> > > system is used, and the fix looks rather simple... ?Can it go to -stable
> > > eventually, even if it is months in the future, after it gets some testing
> > > in .34 ? ? Minor problems are still problems...
> > 
> > Sure, we could do that. There is some stuff in the current .33 kernel,
> > which could go into .32-stable too, if that's useful.
> 
> I think it probably would be useful.  I understand .32 is going to stay with
> us for a long time, so it should get any fixes that have withstood the test
> of time.
> 
> It is very annoying to have subtly different kernel behaviour (from
> mainline) in a long-term stable series...

I've queued up this patch to go into the -stable trees.  Any other
specific one you can think of should also go?

thanks,

greg k-h

Re: Driver-Core: devtmpfs - reset inode permissions before unlinking

[Henrique de Moraes Holschuh]

On Fri, 15 Jan 2010, Greg KH wrote:
> On Sat, Jan 16, 2010 at 12:26:41AM -0200, Henrique de Moraes Holschuh wrote:
> > On Thu, 14 Jan 2010, Kay Sievers wrote:
> > > > That said, this does fix a possible security problem when a misconfigured
> > > > system is used, and the fix looks rather simple... ?Can it go to -stable
> > > > eventually, even if it is months in the future, after it gets some testing
> > > > in .34 ? ? Minor problems are still problems...
> > > 
> > > Sure, we could do that. There is some stuff in the current .33 kernel,
> > > which could go into .32-stable too, if that's useful.
> > 
> > I think it probably would be useful.  I understand .32 is going to stay with
> > us for a long time, so it should get any fixes that have withstood the test
> > of time.
> > 
> > It is very annoying to have subtly different kernel behaviour (from
> > mainline) in a long-term stable series...
> 
> I've queued up this patch to go into the -stable trees.  Any other
> specific one you can think of should also go?

I understand Kay has some patches that are -stable material.  But note that
I'd be happy as long as they _eventually_ go to -stable (the patch in this
thread included), I have nothing against waiting the next mainline kernel
for it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh