fanotify: Make sure to check event_len when copying

PATCH In review Kees Cook 3 participants 4 messages Dec 4, 2018 – Dec 5, 2018

Subsystems (3)

VFS Fsnotify: filesystem notification infrastructure Fanotify

lore

Share & tools

lore

Kees Cook

author

2018-12-04 23:44 UTC #0 lore raw

PATCH 1/1 fanotify: Make sure to check event_len when copying

As a precaution, make sure we check event_len when copying to userspace. Based on old feedback: https://lkml.kernel.org/r/[email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify_user.c +8 -2

index e03be5071362..d9484a0ac6b3 100644
@@ -206,7 +206,7 @@ static int process_access_response(struct fsnotify_group *group,
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
 				  struct fsnotify_event *event,
-				  char __user *buf)
+				  char __user *buf, size_t count)
 {
 	struct fanotify_event_metadata fanotify_event_metadata;
 	struct file *f;

@@ -220,6 +220,12 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 	fd = fanotify_event_metadata.fd;
 	ret = -EFAULT;
+	/*
+	 * Sanity check copy size in case get_one_event() and
+	 * fill_event_metadata() event_len sizes ever get out of sync.
+	 */
+	if (WARN_ON_ONCE(fanotify_event_metadata.event_len > count))
+		goto out_close_fd;
 	if (copy_to_user(buf, &fanotify_event_metadata,
 			 fanotify_event_metadata.event_len))
 		goto out_close_fd;

@@ -295,7 +301,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 			continue;
 		}
 
-		ret = copy_event_to_user(group, kevent, buf);
+		ret = copy_event_to_user(group, kevent, buf, count);
 		if (unlikely(ret == -EOPENSTALE)) {
 			/*
 			 * We cannot report events with stale fd so drop it.

Signature · 5 lines

2.17.1


-- 
Kees Cook

Amir Goldstein

reviewer

2018-12-05 07:15 UTC #1 lore raw

Quoted 4 lines · Kees Cook

As a precaution, make sure we check event_len when copying to userspace. Based on old feedback: https://lkml.kernel.org/r/[email protected]

This precaution serves the new fanotify_event_info_fid format well:
https://marc.info/?l=linux-fsdevel&m=154375073318106&w=2
but also conflicts with my series.

I can weave your patch into my series and add a similar sanity check in copy_fid_to_user() or if Jan applies your patch and has more comments on my v4 series, I'll rebase on top of that.

Thanks,
Amir.

Quoted 46 lines

Signed-off-by Kees Cook <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify_user.c +8 -3

index e03be5071362..d9484a0ac6b3 100644
@@ -206,7 +206,7 @@ static int process_access_response(struct fsnotify_group *group,
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
                                  struct fsnotify_event *event,
-                                 char __user *buf)
+                                 char __user *buf, size_t count)
 {
        struct fanotify_event_metadata fanotify_event_metadata;
        struct file *f;

@@ -220,6 +220,12 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
        fd = fanotify_event_metadata.fd;
        ret = -EFAULT;
+       /*
+        * Sanity check copy size in case get_one_event() and
+        * fill_event_metadata() event_len sizes ever get out of sync.
+        */
+       if (WARN_ON_ONCE(fanotify_event_metadata.event_len > count))
+               goto out_close_fd;
        if (copy_to_user(buf, &fanotify_event_metadata,
                         fanotify_event_metadata.event_len))
                goto out_close_fd;

@@ -295,7 +301,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
                        continue;
                }
 
-               ret = copy_event_to_user(group, kevent, buf);
+               ret = copy_event_to_user(group, kevent, buf, count);
                if (unlikely(ret == -EOPENSTALE)) {
                        /*
                         * We cannot report events with stale fd so drop it.
--

2.17.1

--
Kees Cook

Jan Kara

maintainer

2018-12-05 12:09 UTC #3 lore raw

In reply to Kees Cook

Quoted 4 lines · Kees Cook

As a precaution, make sure we check event_len when copying to userspace. Based on old feedback: https://lkml.kernel.org/r/[email protected]
Signed-off-by Kees Cook <[email protected]>

Thanks for the patch, I've added it to my tree.

Honza

Quoted 45 lines

---
 fs/notify/fanotify/fanotify_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify_user.c +8 -2

index e03be5071362..d9484a0ac6b3 100644
@@ -206,7 +206,7 @@ static int process_access_response(struct fsnotify_group *group,
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
 				  struct fsnotify_event *event,
-				  char __user *buf)
+				  char __user *buf, size_t count)
 {
 	struct fanotify_event_metadata fanotify_event_metadata;
 	struct file *f;

@@ -220,6 +220,12 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 	fd = fanotify_event_metadata.fd;
 	ret = -EFAULT;
+	/*
+	 * Sanity check copy size in case get_one_event() and
+	 * fill_event_metadata() event_len sizes ever get out of sync.
+	 */
+	if (WARN_ON_ONCE(fanotify_event_metadata.event_len > count))
+		goto out_close_fd;
 	if (copy_to_user(buf, &fanotify_event_metadata,
 			 fanotify_event_metadata.event_len))
 		goto out_close_fd;

@@ -295,7 +301,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 			continue;
 		}
 
-		ret = copy_event_to_user(group, kevent, buf);
+		ret = copy_event_to_user(group, kevent, buf, count);
 		if (unlikely(ret == -EOPENSTALE)) {
 			/*
 			 * We cannot report events with stale fd so drop it.

Signature · 5 lines

2.17.1


-- 
Kees Cook

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Jan Kara

maintainer

2018-12-05 12:10 UTC #2 lore raw

In reply to Amir Goldstein

Quoted 6 lines · Amir Goldstein

Quoted 4 lines · Kees Cook
As a precaution, make sure we check event_len when copying to userspace. Based on old feedback: https://lkml.kernel.org/r/[email protected]

Quoted 7 lines

This precaution serves the new fanotify_event_info_fid format well:
https://marc.info/?l=linux-fsdevel&m=154375073318106&w=2
but also conflicts with my series.
I can weave your patch into my series and add a similar sanity check in copy_fid_to_user() or if Jan applies your patch and has more comments on my v4 series, I'll rebase on top of that.

Yeah, I don't think there's any need for you to rebase just now. Either I'll fixup your patch or you'll do the rebase if there are further changes to the series...

Honza

Quoted 50 lines

Thanks,
Amir.

Quoted 46 lines

Signed-off-by Kees Cook <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/notify/fanotify/fanotify_user.c +8 -3

index e03be5071362..d9484a0ac6b3 100644
@@ -206,7 +206,7 @@ static int process_access_response(struct fsnotify_group *group,
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
                                  struct fsnotify_event *event,
-                                 char __user *buf)
+                                 char __user *buf, size_t count)
 {
        struct fanotify_event_metadata fanotify_event_metadata;
        struct file *f;

@@ -220,6 +220,12 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
        fd = fanotify_event_metadata.fd;
        ret = -EFAULT;
+       /*
+        * Sanity check copy size in case get_one_event() and
+        * fill_event_metadata() event_len sizes ever get out of sync.
+        */
+       if (WARN_ON_ONCE(fanotify_event_metadata.event_len > count))
+               goto out_close_fd;
        if (copy_to_user(buf, &fanotify_event_metadata,
                         fanotify_event_metadata.event_len))
                goto out_close_fd;

@@ -295,7 +301,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
                        continue;
                }
 
-               ret = copy_event_to_user(group, kevent, buf);
+               ret = copy_event_to_user(group, kevent, buf, count);
                if (unlikely(ret == -EOPENSTALE)) {
                        /*
                         * We cannot report events with stale fd so drop it.
--

2.17.1

--
Kees Cook

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR