* [PATCH] xfs: change the immutable in xfs_open_by_handle
@ 2013-09-10 18:47 Mark Tinguely
2013-09-10 21:20 ` Dave Chinner
2013-09-11 13:55 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: Mark Tinguely @ 2013-09-10 18:47 UTC (permalink / raw)
To: xfs; +Cc: Greg Banks
[-- Attachment #1: xfs-change-immutable-in-xfs_open_by_handle.patch --]
[-- Type: text/plain, Size: 991 bytes --]
This patch allows clients like DMF to modify an immutable file
without changing the immutable capability on the file, which
would expose the file to change.
This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
so no addition security risk has been introduced.
Signed-off-by: Greg Banks <gbanks@sgi.com>
Singed-off-by: Mark Tinguely <tinguely@sgi.com>
---
fs/xfs/xfs_ioctl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: b/fs/xfs/xfs_ioctl.c
===================================================================
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -237,7 +237,9 @@ xfs_open_by_handle(
goto out_dput;
}
- if ((fmode & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
+ if ((permflag & FMODE_WRITE) &&
+ IS_IMMUTABLE(inode) &&
+ !capable(CAP_LINUX_IMMUTABLE)) {
error = -XFS_ERROR(EACCES);
goto out_dput;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs: change the immutable in xfs_open_by_handle
2013-09-10 18:47 [PATCH] xfs: change the immutable in xfs_open_by_handle Mark Tinguely
@ 2013-09-10 21:20 ` Dave Chinner
2013-09-10 22:29 ` Mark Tinguely
2013-09-11 13:55 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2013-09-10 21:20 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Greg Banks, xfs
On Tue, Sep 10, 2013 at 01:47:20PM -0500, Mark Tinguely wrote:
> This patch allows clients like DMF to modify an immutable file
> without changing the immutable capability on the file, which
> would expose the file to change.
>
> This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
> so no addition security risk has been introduced.
>
> Signed-off-by: Greg Banks <gbanks@sgi.com>
> Singed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> fs/xfs/xfs_ioctl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: b/fs/xfs/xfs_ioctl.c
> ===================================================================
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -237,7 +237,9 @@ xfs_open_by_handle(
> goto out_dput;
> }
>
> - if ((fmode & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
> + if ((permflag & FMODE_WRITE) &&
Why the conversion from fmode to permflag? At minimum, this will
probably throw sparse warnings for comparing a FMODE_* flag against
a variable that is not a fmode_t....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs: change the immutable in xfs_open_by_handle
2013-09-10 21:20 ` Dave Chinner
@ 2013-09-10 22:29 ` Mark Tinguely
0 siblings, 0 replies; 6+ messages in thread
From: Mark Tinguely @ 2013-09-10 22:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: Greg Banks, xfs
On 09/10/13 16:20, Dave Chinner wrote:
> On Tue, Sep 10, 2013 at 01:47:20PM -0500, Mark Tinguely wrote:
>> This patch allows clients like DMF to modify an immutable file
>> without changing the immutable capability on the file, which
>> would expose the file to change.
>>
>> This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
>> so no addition security risk has been introduced.
>>
>> Signed-off-by: Greg Banks <gbanks@sgi.com>
>> Singed-off-by: Mark Tinguely <tinguely@sgi.com>
>> ---
>> fs/xfs/xfs_ioctl.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> Index: b/fs/xfs/xfs_ioctl.c
>> ===================================================================
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -237,7 +237,9 @@ xfs_open_by_handle(
>> goto out_dput;
>> }
>>
>> - if ((fmode & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
>> + if ((permflag & FMODE_WRITE)&&
>
> Why the conversion from fmode to permflag? At minimum, this will
> probably throw sparse warnings for comparing a FMODE_* flag against
> a variable that is not a fmode_t....
>
> Cheers,
>
> Dave.
Yes, my mistake - a case of posting a Linux 3.0.X based patch.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: change the immutable in xfs_open_by_handle
2013-09-10 18:47 [PATCH] xfs: change the immutable in xfs_open_by_handle Mark Tinguely
2013-09-10 21:20 ` Dave Chinner
@ 2013-09-11 13:55 ` Christoph Hellwig
2013-09-11 14:22 ` Mark Tinguely
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2013-09-11 13:55 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Greg Banks, xfs
On Tue, Sep 10, 2013 at 01:47:20PM -0500, Mark Tinguely wrote:
> This patch allows clients like DMF to modify an immutable file
> without changing the immutable capability on the file, which
> would expose the file to change.
>
> This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
> so no addition security risk has been introduced.
The immutable flag means that the file can't be modified, and
CAP_LINUX_IMMUTABLE allows setting/removing that restriction, but not
ignoring it.
So: NAK, this is a change in semantics and long-standing behaviour.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: change the immutable in xfs_open_by_handle
2013-09-11 13:55 ` Christoph Hellwig
@ 2013-09-11 14:22 ` Mark Tinguely
2013-09-11 15:50 ` Matthias Schniedermeyer
0 siblings, 1 reply; 6+ messages in thread
From: Mark Tinguely @ 2013-09-11 14:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Greg Banks, xfs
On 09/11/13 08:55, Christoph Hellwig wrote:
> On Tue, Sep 10, 2013 at 01:47:20PM -0500, Mark Tinguely wrote:
>> This patch allows clients like DMF to modify an immutable file
>> without changing the immutable capability on the file, which
>> would expose the file to change.
>>
>> This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
>> so no addition security risk has been introduced.
>
> The immutable flag means that the file can't be modified, and
> CAP_LINUX_IMMUTABLE allows setting/removing that restriction, but not
> ignoring it.
>
> So: NAK, this is a change in semantics and long-standing behaviour.
>
As you said, the CAP_LINUX_IMMUTABLE allows the holder of the capability
to turn on/off the restriction. The holder of CAP_LINUX_IMMUTABLE could
turn off immutability, modify it and then turn it back on, but during
that window, others could modify it too which may be more undesirable
than changing the behavior.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: change the immutable in xfs_open_by_handle
2013-09-11 14:22 ` Mark Tinguely
@ 2013-09-11 15:50 ` Matthias Schniedermeyer
0 siblings, 0 replies; 6+ messages in thread
From: Matthias Schniedermeyer @ 2013-09-11 15:50 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Christoph Hellwig, Greg Banks, xfs
On 11.09.2013 09:22, Mark Tinguely wrote:
> On 09/11/13 08:55, Christoph Hellwig wrote:
> >On Tue, Sep 10, 2013 at 01:47:20PM -0500, Mark Tinguely wrote:
> >>This patch allows clients like DMF to modify an immutable file
> >>without changing the immutable capability on the file, which
> >>would expose the file to change.
> >>
> >>This patch is restricted to holders of the CAP_LINUX_IMMUTABLE,
> >>so no addition security risk has been introduced.
> >
> >The immutable flag means that the file can't be modified, and
> >CAP_LINUX_IMMUTABLE allows setting/removing that restriction, but not
> >ignoring it.
> >
> >So: NAK, this is a change in semantics and long-standing behaviour.
> >
>
> As you said, the CAP_LINUX_IMMUTABLE allows the holder of the
> capability to turn on/off the restriction. The holder of
> CAP_LINUX_IMMUTABLE could turn off immutability, modify it and then
> turn it back on, but during that window, others could modify it too
> which may be more undesirable than changing the behavior.
It's like setting a file you own "444" only because you CAN set it to
666 doesn't mean that the check should be short-circuted.
$ touch test
$ chmod 444 test
$ echo 'Hallo' > test
-bash: test: Permission denied
--
Matthias
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-11 15:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-10 18:47 [PATCH] xfs: change the immutable in xfs_open_by_handle Mark Tinguely
2013-09-10 21:20 ` Dave Chinner
2013-09-10 22:29 ` Mark Tinguely
2013-09-11 13:55 ` Christoph Hellwig
2013-09-11 14:22 ` Mark Tinguely
2013-09-11 15:50 ` Matthias Schniedermeyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox