2011-03-24 12:59:49

by [email protected]

[permalink] [raw]
Subject: [Patch 1/1] fsnotify,fanotify: adding flag for execution

From: Jozef Kralik <[email protected]>

This patch add flag FS_OPENEXEC[FAN_OPENEXEC] to event
FS_OPEN_PERM[FAN_OPEN_PERM], when file is opened with flag FMODE_EXEC.

Signed-off-by: Jozef Kralik <[email protected]>
---
Example:
if (metadata->mask & FAN_OPEN_PERM)
if (metadata->mask & FAN_OPENEXEC)
printf("file was executed");
else
printf("file was opened");

Patch for kernel: 2.6.38
Developed kernel: 2.6.37-rc4 with patch-v2.6.37-rc4-next-20101201
Tested kernel: 2.6.38

diffstat -p1 ./patch_exec_2.6.38.diff
fs/notify/fanotify/fanotify.c | 1 +
fs/notify/fsnotify.c | 2 +-
include/linux/fanotify.h | 4 +++-
include/linux/fsnotify.h | 9 ++++++---
include/linux/fsnotify_backend.h | 4 +++-
5 files changed, 14 insertions(+), 6 deletions(-)

diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/fs/notify/fanotify/fanotify.c linux-2.6.38-dev/fs/notify/fanotify/fanotify.c
--- linux-2.6.38/fs/notify/fanotify/fanotify.c 2011-03-15 02:20:32.000000000 +0100
+++ linux-2.6.38-dev/fs/notify/fanotify/fanotify.c 2011-03-24 12:34:40.182283000 +0100
@@ -131,6 +131,7 @@ static int fanotify_handle_event(struct
BUILD_BUG_ON(FAN_CLOSE_NOWRITE != FS_CLOSE_NOWRITE);
BUILD_BUG_ON(FAN_CLOSE_WRITE != FS_CLOSE_WRITE);
BUILD_BUG_ON(FAN_OPEN != FS_OPEN);
+ BUILD_BUG_ON(FAN_OPENEXEC != FS_OPENEXEC);
BUILD_BUG_ON(FAN_EVENT_ON_CHILD != FS_EVENT_ON_CHILD);
BUILD_BUG_ON(FAN_Q_OVERFLOW != FS_Q_OVERFLOW);
BUILD_BUG_ON(FAN_OPEN_PERM != FS_OPEN_PERM);
diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/fs/notify/fsnotify.c linux-2.6.38-dev/fs/notify/fsnotify.c
--- linux-2.6.38/fs/notify/fsnotify.c 2011-03-15 02:20:32.000000000 +0100
+++ linux-2.6.38-dev/fs/notify/fsnotify.c 2011-03-24 12:34:40.186283000 +0100
@@ -299,7 +299,7 @@ static __init int fsnotify_init(void)
{
int ret;

- BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23);
+ BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 24);

ret = init_srcu_struct(&fsnotify_mark_srcu);
if (ret)
diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fanotify.h linux-2.6.38-dev/include/linux/fanotify.h
--- linux-2.6.38/include/linux/fanotify.h 2011-03-15 02:20:32.000000000 +0100
+++ linux-2.6.38-dev/include/linux/fanotify.h 2011-03-24 12:34:40.190283001 +0100
@@ -9,6 +9,7 @@
#define FAN_CLOSE_WRITE 0x00000008 /* Writtable file closed */
#define FAN_CLOSE_NOWRITE 0x00000010 /* Unwrittable file closed */
#define FAN_OPEN 0x00000020 /* File was opened */
+#define FAN_OPENEXEC 0x00001000 /* File had exec flag */

#define FAN_Q_OVERFLOW 0x00004000 /* Event queued overflowed */

@@ -81,7 +82,8 @@

#define FAN_ALL_OUTGOING_EVENTS (FAN_ALL_EVENTS |\
FAN_ALL_PERM_EVENTS |\
- FAN_Q_OVERFLOW)
+ FAN_Q_OVERFLOW |\
+ FAN_OPENEXEC)

#define FANOTIFY_METADATA_VERSION 3

diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fsnotify_backend.h linux-2.6.38-dev/include/linux/fsnotify_backend.h
--- linux-2.6.38/include/linux/fsnotify_backend.h 2011-03-15 02:20:32.000000000 +0100
+++ linux-2.6.38-dev/include/linux/fsnotify_backend.h 2011-03-24 12:34:40.262283001 +0100
@@ -36,6 +36,7 @@
#define FS_DELETE 0x00000200 /* Subfile was deleted */
#define FS_DELETE_SELF 0x00000400 /* Self was deleted */
#define FS_MOVE_SELF 0x00000800 /* Self was moved */
+#define FS_OPENEXEC 0x00001000 /* File had exec flag */

#define FS_UNMOUNT 0x00002000 /* inode on umount fs */
#define FS_Q_OVERFLOW 0x00004000 /* Event queued overflowed */
@@ -73,7 +74,8 @@
FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
FS_OPEN_PERM | FS_ACCESS_PERM | FS_EXCL_UNLINK | \
FS_ISDIR | FS_IN_ONESHOT | FS_DN_RENAME | \
- FS_DN_MULTISHOT | FS_EVENT_ON_CHILD)
+ FS_DN_MULTISHOT | FS_EVENT_ON_CHILD | \
+ FS_OPENEXEC)

struct fsnotify_group;
struct fsnotify_event;
diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fsnotify.h linux-2.6.38-dev/include/linux/fsnotify.h
--- linux-2.6.38/include/linux/fsnotify.h 2011-03-15 02:20:32.000000000 +0100
+++ linux-2.6.38-dev/include/linux/fsnotify.h 2011-03-24 12:34:40.274283001 +0100
@@ -45,12 +45,15 @@ static inline int fsnotify_perm(struct f
return 0;
if (!(mask & (MAY_READ | MAY_OPEN)))
return 0;
- if (mask & MAY_OPEN)
+ if (mask & MAY_OPEN) {
fsnotify_mask = FS_OPEN_PERM;
- else if (mask & MAY_READ)
+ if (file->f_flags & FMODE_EXEC)
+ fsnotify_mask |= FS_OPENEXEC;
+ } else if (mask & MAY_READ) {
fsnotify_mask = FS_ACCESS_PERM;
- else
+ } else {
BUG();
+ }

ret = fsnotify_parent(path, NULL, fsnotify_mask);
if (ret)


2011-03-24 15:31:51

by Eric Paris

[permalink] [raw]
Subject: Re: [Patch 1/1] fsnotify,fanotify: adding flag for execution

On Thu, 2011-03-24 at 12:49 +0000, [email protected] wrote:
> From: Jozef Kralik <[email protected]>
>
> This patch add flag FS_OPENEXEC[FAN_OPENEXEC] to event
> FS_OPEN_PERM[FAN_OPEN_PERM], when file is opened with flag FMODE_EXEC.
>
> Signed-off-by: Jozef Kralik <[email protected]>

I keep waffling back and forth on this patch (for months now) mostly
because of the fact that I'm scared it will give people a false sense
that they will get notification of all files that might be executed. I
don't understand the use case at all so I don't know if it good idea to
expose such notifications....

I'm just having a hard time deciding if I'm comfortable solving half of
a problem... What exactly is your goal here and how do you see others
using it, usefully.

-Eric

> ---
> Example:
> if (metadata->mask & FAN_OPEN_PERM)
> if (metadata->mask & FAN_OPENEXEC)
> printf("file was executed");
> else
> printf("file was opened");
>
> Patch for kernel: 2.6.38
> Developed kernel: 2.6.37-rc4 with patch-v2.6.37-rc4-next-20101201
> Tested kernel: 2.6.38
>
> diffstat -p1 ./patch_exec_2.6.38.diff
> fs/notify/fanotify/fanotify.c | 1 +
> fs/notify/fsnotify.c | 2 +-
> include/linux/fanotify.h | 4 +++-
> include/linux/fsnotify.h | 9 ++++++---
> include/linux/fsnotify_backend.h | 4 +++-
> 5 files changed, 14 insertions(+), 6 deletions(-)
>
> diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/fs/notify/fanotify/fanotify.c linux-2.6.38-dev/fs/notify/fanotify/fanotify.c
> --- linux-2.6.38/fs/notify/fanotify/fanotify.c 2011-03-15 02:20:32.000000000 +0100
> +++ linux-2.6.38-dev/fs/notify/fanotify/fanotify.c 2011-03-24 12:34:40.182283000 +0100
> @@ -131,6 +131,7 @@ static int fanotify_handle_event(struct
> BUILD_BUG_ON(FAN_CLOSE_NOWRITE != FS_CLOSE_NOWRITE);
> BUILD_BUG_ON(FAN_CLOSE_WRITE != FS_CLOSE_WRITE);
> BUILD_BUG_ON(FAN_OPEN != FS_OPEN);
> + BUILD_BUG_ON(FAN_OPENEXEC != FS_OPENEXEC);
> BUILD_BUG_ON(FAN_EVENT_ON_CHILD != FS_EVENT_ON_CHILD);
> BUILD_BUG_ON(FAN_Q_OVERFLOW != FS_Q_OVERFLOW);
> BUILD_BUG_ON(FAN_OPEN_PERM != FS_OPEN_PERM);
> diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/fs/notify/fsnotify.c linux-2.6.38-dev/fs/notify/fsnotify.c
> --- linux-2.6.38/fs/notify/fsnotify.c 2011-03-15 02:20:32.000000000 +0100
> +++ linux-2.6.38-dev/fs/notify/fsnotify.c 2011-03-24 12:34:40.186283000 +0100
> @@ -299,7 +299,7 @@ static __init int fsnotify_init(void)
> {
> int ret;
>
> - BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23);
> + BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 24);
>
> ret = init_srcu_struct(&fsnotify_mark_srcu);
> if (ret)
> diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fanotify.h linux-2.6.38-dev/include/linux/fanotify.h
> --- linux-2.6.38/include/linux/fanotify.h 2011-03-15 02:20:32.000000000 +0100
> +++ linux-2.6.38-dev/include/linux/fanotify.h 2011-03-24 12:34:40.190283001 +0100
> @@ -9,6 +9,7 @@
> #define FAN_CLOSE_WRITE 0x00000008 /* Writtable file closed */
> #define FAN_CLOSE_NOWRITE 0x00000010 /* Unwrittable file closed */
> #define FAN_OPEN 0x00000020 /* File was opened */
> +#define FAN_OPENEXEC 0x00001000 /* File had exec flag */
>
> #define FAN_Q_OVERFLOW 0x00004000 /* Event queued overflowed */
>
> @@ -81,7 +82,8 @@
>
> #define FAN_ALL_OUTGOING_EVENTS (FAN_ALL_EVENTS |\
> FAN_ALL_PERM_EVENTS |\
> - FAN_Q_OVERFLOW)
> + FAN_Q_OVERFLOW |\
> + FAN_OPENEXEC)
>
> #define FANOTIFY_METADATA_VERSION 3
>
> diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fsnotify_backend.h linux-2.6.38-dev/include/linux/fsnotify_backend.h
> --- linux-2.6.38/include/linux/fsnotify_backend.h 2011-03-15 02:20:32.000000000 +0100
> +++ linux-2.6.38-dev/include/linux/fsnotify_backend.h 2011-03-24 12:34:40.262283001 +0100
> @@ -36,6 +36,7 @@
> #define FS_DELETE 0x00000200 /* Subfile was deleted */
> #define FS_DELETE_SELF 0x00000400 /* Self was deleted */
> #define FS_MOVE_SELF 0x00000800 /* Self was moved */
> +#define FS_OPENEXEC 0x00001000 /* File had exec flag */
>
> #define FS_UNMOUNT 0x00002000 /* inode on umount fs */
> #define FS_Q_OVERFLOW 0x00004000 /* Event queued overflowed */
> @@ -73,7 +74,8 @@
> FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
> FS_OPEN_PERM | FS_ACCESS_PERM | FS_EXCL_UNLINK | \
> FS_ISDIR | FS_IN_ONESHOT | FS_DN_RENAME | \
> - FS_DN_MULTISHOT | FS_EVENT_ON_CHILD)
> + FS_DN_MULTISHOT | FS_EVENT_ON_CHILD | \
> + FS_OPENEXEC)
>
> struct fsnotify_group;
> struct fsnotify_event;
> diff -uprN -X linux-2.6.38/Documentation/dontdiff linux-2.6.38/include/linux/fsnotify.h linux-2.6.38-dev/include/linux/fsnotify.h
> --- linux-2.6.38/include/linux/fsnotify.h 2011-03-15 02:20:32.000000000 +0100
> +++ linux-2.6.38-dev/include/linux/fsnotify.h 2011-03-24 12:34:40.274283001 +0100
> @@ -45,12 +45,15 @@ static inline int fsnotify_perm(struct f
> return 0;
> if (!(mask & (MAY_READ | MAY_OPEN)))
> return 0;
> - if (mask & MAY_OPEN)
> + if (mask & MAY_OPEN) {
> fsnotify_mask = FS_OPEN_PERM;
> - else if (mask & MAY_READ)
> + if (file->f_flags & FMODE_EXEC)
> + fsnotify_mask |= FS_OPENEXEC;
> + } else if (mask & MAY_READ) {
> fsnotify_mask = FS_ACCESS_PERM;
> - else
> + } else {
> BUG();
> + }
>
> ret = fsnotify_parent(path, NULL, fsnotify_mask);
> if (ret)
>

2011-03-25 01:18:46

by Linus Torvalds

[permalink] [raw]
Subject: Re: [Patch 1/1] fsnotify,fanotify: adding flag for execution

On Thu, Mar 24, 2011 at 8:30 AM, Eric Paris <[email protected]> wrote:
>
> I keep waffling back and forth on this patch (for months now) mostly
> because of the fact that I'm scared it will give people a false sense
> that they will get notification of all files that might be executed. ?I
> don't understand the use case at all so I don't know if it good idea to
> expose such notifications....

We have indeed historically had those kinds of bugs. For example,
having 'noexec' disable execve() on files, but *not* disabling using
them as LD_PRELOAD=xyz things where they aren't the target of an
execve(), but the code in them is run anyway (thanks to just an
executable mmap(), or even a "read()" into a data segment that is
executable)

So I don't know what makes "executed" different from "read". Because
at some point we really cannot tell the difference.

The one special thing about execve() is that it can execute something
even when it's not readable. But people who have depended on that as a
security feature have always been disappointed (ie just execve it and
then use ptrace to read the contents of the file _anyway_). So once
again it's not at all clear that "execute" should ever be considered
to be anything but "read".

Linus

2011-03-25 09:56:35

by Alan

[permalink] [raw]
Subject: Re: [Patch 1/1] fsnotify,fanotify: adding flag for execution

> We have indeed historically had those kinds of bugs. For example,
> having 'noexec' disable execve() on files, but *not* disabling using
> them as LD_PRELOAD=xyz things where they aren't the target of an
> execve(), but the code in them is run anyway (thanks to just an
> executable mmap(), or even a "read()" into a data segment that is
> executable)

Actually there is a difference

> So I don't know what makes "executed" different from "read". Because
> at some point we really cannot tell the difference.

And this isn't quite true

> The one special thing about execve() is that it can execute something
> even when it's not readable. But people who have depended on that as a
> security feature have always been disappointed (ie just execve it and
> then use ptrace to read the contents of the file _anyway_). So once

And ptrace covers this

The specific case that has always been the one Unix has cared about is
'exec but no read bit', and execve() covers that correctly, including
stopping it being dumpable thus blocking ptrace.

Various standard Unix tricks rely upon this behaviour, notably the cd
into a directory whose name you can't determine to access "protected" by
writable stuff. All of the latter is really about suid apps though.

Alan