fs: avoid fdput() after failed fdget()

PATCH Superseded Shuah Khan 4 participants 11 messages May 12, 2020 – May 22, 2020

Subsystems (3)

VFS Security (LSM) exec and binfmt

lore

Share & tools

lore

Shuah Khan

author

2020-05-12 19:43 UTC #0 lore raw

v2 cover fs: avoid fdput() after failed fdget()

While debugging an unrelated problem, I noticed these two cases fdput() is called after failed fdget() while reviewing at all the fdget() and fdput() paths in the kernel.

Changes since v1:
Patch 1:
Changed to address review comments to refine the code for improved readability in addition to the change to avoid fdput() on failed fdget()
Patch 2:
No change to v1. Including it in the series to keep the patches together.

Shuah Khan (2):
fs: avoid fdput() after failed fdget() in ksys_sync_file_range()
fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

 fs/exec.c |  2 +-
 fs/sync.c | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

Signature · 1 line

2.25.1

Shuah Khan

author

2020-05-12 19:43 UTC #1 lore raw

PATCH v2 1/2 fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Fix ksys_sync_file_range() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags.

Change it anyway since failed fdget() doesn't require a fdput(). Refine the code path a bit for it to read more clearly.
Reference: commit 22f96b3808c1 ("fs: add sync_file_range() helper")

Signed-off-by Shuah Khan <[email protected]>

---
 fs/sync.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

fs/sync.c +6 -6

index 4d1ff010bc5a..300ca73ec87c 100644
@@ -364,15 +364,15 @@ int sync_file_range(struct file *file, loff_t offset, loff_t nbytes,
 int ksys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
 			 unsigned int flags)
 {
-	int ret;
-	struct fd f;
+	int ret = -EBADF;
+	struct fd f = fdget(fd);
 
-	ret = -EBADF;
-	f = fdget(fd);
-	if (f.file)
-		ret = sync_file_range(f.file, offset, nbytes, flags);
+	if (!f.file)
+		goto out;
 
+	ret = sync_file_range(f.file, offset, nbytes, flags);
 	fdput(f);
+out:
 	return ret;
 }

Signature · 1 line

2.25.1

Al Viro

maintainer

2020-05-13 05:46 UTC #2 lore raw

Re: [PATCH v2 1/2] fs: avoid fdput() after failed fdget() in ksys_sync_file_range()

Quoted 10 lines · Shuah Khan

fs/sync.c +2 -3

@@ -364,15 +364,15 @@ int sync_file_range(struct file *file, loff_t offset, loff_t nbytes,
 int ksys_sync_file_range(int fd, loff_t offset, loff_t nbytes,
 			 unsigned int flags)
 {
-	int ret;
-	struct fd f;
+	int ret = -EBADF;
+	struct fd f = fdget(fd);
 
-	ret = -EBADF;

fs/sync.c int sync_file_range(struct file *file, loff_t offset, loff_t nbytes, · line 376

Quoted 10 lines

fs/sync.c +4 -3

-	f = fdget(fd);
-	if (f.file)
-		ret = sync_file_range(f.file, offset, nbytes, flags);
+	if (!f.file)
+		goto out;
 
+	ret = sync_file_range(f.file, offset, nbytes, flags);
 	fdput(f);
+out:
 	return ret;

IDGI... What's the point of that goto out, when it leads straight to return?

Shuah Khan

author

2020-05-12 19:43 UTC #3 lore raw

PATCH v2 2/2 fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

In reply to Shuah Khan

Fix kernel_read_file_from_fd() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput().

This was introduced in a commit that added kernel_read_file_from_fd() as a wrapper for the VFS common kernel_read_file().

Fixes b844f0ecbc56 ("vfs: define kernel_copy_file_from_fd()")

Signed-off-by Shuah Khan <[email protected]>

---
 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/exec.c +1 -1

index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
 		goto out;
 
 	ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
 	fdput(f);
+out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);

Signature · 1 line

2.25.1

Al Viro

maintainer

2020-05-13 05:49 UTC #4 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Quoted 19 lines · Shuah Khan

Fix kernel_read_file_from_fd() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput().
This was introduced in a commit that added kernel_read_file_from_fd() as a wrapper for the VFS common kernel_read_file().
Fixes b844f0ecbc56 ("vfs: define kernel_copy_file_from_fd()")
Signed-off-by Shuah Khan <[email protected]>
---
 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/exec.c +0 -0
index 06b4c550af5d..ea24bdce939d 100644

fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026

Quoted 7 lines

fs/exec.c +1 -1

@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
 		goto out;
 
 	ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
 	fdput(f);
+out:
 	return ret;

Again, that goto is a pointless obfuscation; just return -EBADF and be done with that.

Incidentally, why is that thing exported?

Luis Chamberlain

2020-05-13 13:13 UTC #5 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Quoted 8 lines · Al Viro

Quoted 7 lines · Shuah Khan

fs/exec.c +0 -0

index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
 		goto out;

Quoted 7 lines

fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026
Quoted 5 lines
fs/exec.c +1 -1
 	ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
 	fdput(f);
+out:
 	return ret;
Incidentally, why is that thing exported?

Both kernel_read_file_from_fd() and kernel_read_file() are exported because they have users, however kernel_read_file() only has security stuff as a user. Do we want to get rid of the lsm hook for it?

I also have some non-posted patches which tucks away these kernel_read*() exports under a symbol namespace, to avoid wide-spread use / abuse on areas in the kernel, so I'd be happy to take this on if we want to remove it export / lsm hook as part of my series. I did this as there is another series of patches for a new driver which extend these family of functions with a now pread() variant....

Luis

Luis Chamberlain

2020-05-13 14:19 UTC #6 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Quoted 18 lines · Luis Chamberlain

Quoted 8 lines · Al Viro
Quoted 7 lines · Shuah Khan
fs/exec.c +0 -0
index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
            goto out;
 
Quoted 7 lines
fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026
Quoted 5 lines
fs/exec.c +1 -1
    ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
    fdput(f);
+out:
    return ret;
Incidentally, why is that thing exported?

Quoted 3 lines

Both kernel_read_file_from_fd() and kernel_read_file() are exported because they have users, however kernel_read_file() only has security stuff as a user. Do we want to get rid of the lsm hook for it?

Alright, yeah just the export needs to be removed. I have a patch series dealing with these callers so will add it to my queue.

Luis

Scott Branden

2020-05-22 22:00 UTC #7 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Hi Luis,

Quoted 16 lines · Luis Chamberlain

Quoted 15 lines · Luis Chamberlain
Quoted 14 lines · Al Viro
fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026
Quoted 12 lines · Shuah Khan
fs/exec.c +1 -1
index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
             goto out;
 
     ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
     fdput(f);
+out:
     return ret;
Incidentally, why is that thing exported?

Quoted 5 lines

Quoted 3 lines
Both kernel_read_file_from_fd() and kernel_read_file() are exported because they have users, however kernel_read_file() only has security stuff as a user. Do we want to get rid of the lsm hook for it?
Alright, yeah just the export needs to be removed. I have a patch series dealing with these callers so will add it to my queue.

When will these changes make it into linux-next?
It is difficult for me to complete my patch series without these other misc. changes in place.

Quoted 2 lines

Luis

Regards,
Scott

Scott Branden

2020-05-22 22:15 UTC #8 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Quoted 20 lines · Scott Branden

Hi Luis,
Quoted 17 lines · Luis Chamberlain
Quoted 16 lines · Luis Chamberlain
Quoted 15 lines · Al Viro
fs/exec.c int kernel_read_file_from_fd(int fd, void · line 1027
Quoted 13 lines · Shuah Khan
fs/exec.c +1 -1
index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void 
 **buf, loff_t *size, loff_t max_size,
              goto out;
 
      ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
      fdput(f);
+out:
      return ret;
Incidentally, why is that thing exported?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Both kernel_read_file_from_fd() and kernel_read_file() are exported because they have users, however kernel_read_file() only has security stuff as a user. Do we want to get rid of the lsm hook for it?
Alright, yeah just the export needs to be removed. I have a patch series dealing with these callers so will add it to my queue.
When will these changes make it into linux-next?
It is difficult for me to complete my patch series without these other misc. changes in place.

Sorry, I see the patch series is still being worked on (missing changelog, comments, etc). Hopefully the patches stabilize so I can apply my changes on top fairly soon.

Quoted 4 lines

Quoted 2 lines
Luis
Regards,
Scott

Luis Chamberlain

2020-05-22 22:47 UTC #9 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

Quoted 29 lines · Scott Branden

Quoted 26 lines · Scott Branden
Hi Luis,
Quoted 23 lines · Luis Chamberlain
Quoted 16 lines · Luis Chamberlain
Quoted 9 lines · Al Viro
Quoted 8 lines · Shuah Khan
fs/exec.c +0 -0
index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int
 fd, void **buf, loff_t *size, loff_t max_size,
              goto out;
 
Quoted 6 lines
fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026
Quoted 5 lines
fs/exec.c +1 -1
      ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
      fdput(f);
+out:
      return ret;
Incidentally, why is that thing exported?
Quoted 3 lines
Both kernel_read_file_from_fd() and kernel_read_file() are exported because they have users, however kernel_read_file() only has security stuff as a user. Do we want to get rid of the lsm hook for it?
Alright, yeah just the export needs to be removed. I have a patch series dealing with these callers so will add it to my queue.

Quoted 7 lines

Quoted 3 lines
When will these changes make it into linux-next?
It is difficult for me to complete my patch series without these other misc. changes in place.
Sorry, I see the patch series is still being worked on (missing changelog, comments, etc).
Hopefully the patches stabilize so I can apply my changes on top fairly soon.

Yeah I have to redo that series to take into account feedback. I'll be sure cc you on that. I have a few other series to attend to before that, so I think this will take a week.

Luis

Shuah Khan

author

2020-05-13 17:56 UTC #10 lore raw

Re: [PATCH v2 2/2] fs: avoid fdput() after failed fdget() in kernel_read_file_from_fd()

In reply to Al Viro

Quoted 22 lines · Al Viro

Quoted 21 lines · Shuah Khan
Fix kernel_read_file_from_fd() to avoid fdput() after a failed fdget(). fdput() doesn't do fput() on this file since FDPUT_FPUT isn't set in fd.flags. Fix it anyway since failed fdget() doesn't require a fdput().
This was introduced in a commit that added kernel_read_file_from_fd() as a wrapper for the VFS common kernel_read_file().
Fixes b844f0ecbc56 ("vfs: define kernel_copy_file_from_fd()")
Signed-off-by Shuah Khan <[email protected]>
---
  fs/exec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fs/exec.c +0 -0
index 06b4c550af5d..ea24bdce939d 100644
@@ -1021,8 +1021,8 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
  		goto out;
 

Quoted 9 lines

fs/exec.c int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, · line 1026
Quoted 5 lines
fs/exec.c +1 -1
  	ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
  	fdput(f);
+out:
  	return ret;
Again, that goto is a pointless obfuscation; just return -EBADF and be done with that.

My reasoning is if this routine ends up growing it might be useful to handle the return this way.

In any case, I will send v3 for both of these patches.

thanks,
-- Shuah