Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Kees,

one more comment below.

On 2020-07-07 9:01 p.m., Scott Branden wrote:



On 2020-07-07 4:56 p.m., Kees Cook wrote:

On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:

Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.

Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
---
  fs/exec.c                        | 93 ++++++++++++++++++++++++-------- 
  include/linux/kernel_read_file.h | 17 ++++++
  2 files changed, 87 insertions(+), 23 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 4ea87db5e4d5..e6a8a65f7478 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -928,10 +928,14 @@ struct file *open_exec(const char *name)
  }
  EXPORT_SYMBOL(open_exec);
  -int kernel_read_file(struct file *file, void **buf, loff_t *size,
-             loff_t max_size, enum kernel_read_file_id id)
-{
-    loff_t i_size, pos;
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
+              loff_t max_size, loff_t pos,
+              enum kernel_read_file_id id)
+{
+    loff_t alloc_size;
+    loff_t buf_pos;
+    loff_t read_end;
+    loff_t i_size;
      ssize_t bytes = 0;
      int ret;
  @@ -951,21 +955,32 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, 
          ret = -EINVAL;
          goto out;
      }
-    if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+
+    /* Default read to end of file */
+    read_end = i_size;
+
+    /* Allow reading partial portion of file */
+    if ((id == READING_FIRMWARE_PARTIAL_READ) &&
+        (i_size > (pos + max_size)))
+        read_end = pos + max_size;

There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).
There are other signals other than the fact that kernel_read_file requires the entire file to be read while kernel_pread_file allows partial files to be read. So if you do a pread at pos = 0 you need another key to indicate it is "ok" if max_size < i_size. If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share 99% of the code between kernel_read_file and kernel_pread_file then I need to add another parameter to a common function called between these functions.  And adding another parameter was rejected previously in the review as a "swiss army knife approach" by another reviewer.  I am happy to add it back in because it is necessary to share code and differentiate whether we are performing a partial read or not. 


+
+    alloc_size = read_end - pos;
+    if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) { 
          ret = -EFBIG;
          goto out;
      }
  -    if (id != READING_FIRMWARE_PREALLOC_BUFFER)
-        *buf = vmalloc(i_size);
+    if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+        (id != READING_FIRMWARE_PREALLOC_BUFFER))
+        *buf = vmalloc(alloc_size);
      if (!*buf) {
          ret = -ENOMEM;
          goto out;
      }

The id usage here was a mistake in upstream, and the series I sent is
trying to clean that up.
I see that cleanup and it works fine with the pread.  Other than I need some sort of key to share code and indicate whether it is "ok" to do a partial read of the file or not. 

Greg, it seems this series is going to end up in your tree due to it
being drivers/misc? I guess I need to direct my series to Greg then, but
get LSM folks Acks.


  -    pos = 0;
-    while (pos < i_size) {
-        bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+    buf_pos = 0;
+    while (pos < read_end) {
+        bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos); 
          if (bytes < 0) {
              ret = bytes;
              goto out_free;
@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, 
            if (bytes == 0)
              break;
+
+        buf_pos += bytes;
      }
  -    if (pos != i_size) {
+    if (pos != read_end) {
          ret = -EIO;
          goto out_free;
      }
  -    ret = security_kernel_post_read_file(file, *buf, i_size, id);
+    ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
      if (!ret)
          *size = pos;

This call cannot be inside kernel_pread_file(): any future LSMs will see
a moving window of contents, etc. It'll need to be in kernel_read_file()
proper.
If IMA still passes (after testing my next patch series with your changes and my modifications) 
I will need some more help here.



    out_free:
      if (ret < 0) {
-        if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+        if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+            (id != READING_FIRMWARE_PREALLOC_BUFFER)) {
              vfree(*buf);
              *buf = NULL;
          }
@@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size, 
      allow_write_access(file);
      return ret;
  }
+
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
+             loff_t max_size, enum kernel_read_file_id id)
+{
+    return kernel_pread_file(file, buf, size, max_size, 0, id);
+}
  EXPORT_SYMBOL_GPL(kernel_read_file);
  -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, 
-                   loff_t max_size, enum kernel_read_file_id id)
+int kernel_pread_file_from_path(const char *path, void **buf,
+                loff_t *size,
+                loff_t max_size, loff_t pos,
+                enum kernel_read_file_id id)
  {
      struct file *file;
      int ret;
@@ -1011,15 +1037,22 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, 
      if (IS_ERR(file))
          return PTR_ERR(file);
  -    ret = kernel_read_file(file, buf, size, max_size, id);
+    ret = kernel_pread_file(file, buf, size, max_size, pos, id);
      fput(file);
      return ret;
  }
+
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size, 
+                   loff_t max_size, enum kernel_read_file_id id)
+{
+    return kernel_pread_file_from_path(path, buf, size, max_size, 0, id); 
+}
  EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
  -int kernel_read_file_from_path_initns(const char *path, void **buf,
-                      loff_t *size, loff_t max_size,
-                      enum kernel_read_file_id id)
+int kernel_pread_file_from_path_initns(const char *path, void **buf,
+                       loff_t *size,
+                       loff_t max_size, loff_t pos,
+                       enum kernel_read_file_id id)
  {
      struct file *file;
      struct path root;
@@ -1037,14 +1070,22 @@ int kernel_read_file_from_path_initns(const char *path, void **buf, 
      if (IS_ERR(file))
          return PTR_ERR(file);
  -    ret = kernel_read_file(file, buf, size, max_size, id);
+    ret = kernel_pread_file(file, buf, size, max_size, pos, id);
      fput(file);
      return ret;
  }
+
+int kernel_read_file_from_path_initns(const char *path, void **buf,
+                      loff_t *size, loff_t max_size,
+                      enum kernel_read_file_id id)
+{
+    return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id); 
+}
  EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
  -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, 
-                 enum kernel_read_file_id id)
+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
+                  loff_t max_size, loff_t pos,
+                  enum kernel_read_file_id id)
  {
      struct fd f = fdget(fd);
      int ret = -EBADF;
@@ -1052,11 +1093,17 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, 
      if (!f.file)
          goto out;
  -    ret = kernel_read_file(f.file, buf, size, max_size, id);
+    ret = kernel_pread_file(f.file, buf, size, max_size, pos, id);
  out:
      fdput(f);
      return ret;
  }
+
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size, 
+                 enum kernel_read_file_id id)
+{
+    return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
+}
  EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);

For each of these execution path, the mapping to LSM hooks is:

- all path must call security_kernel_read_file(file, id) before reading
   (this appears to be fine as-is in your series).

- anything doing a "full" read needs to call
   security_kernel_post_read_file() with the file and full buffer, size,
   etc (so all the kernel_read_file*() paths). I imagine instead of
   adding 3 copy/pasted versions of this, it may be possible to refactor
   the helpers into a single core "full" caller that takes struct file,
   or doing some logic in kernel_pread_file() that notices it has the
   entire file in the buffer and doing the call then.
   As an example of what I mean about doing the call, here's how I might
   imagine it for one of the paths if it took struct file:
int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size, 
                   loff_t max_size, enum kernel_read_file_id id)
{
    int ret;

    ret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
    if (ret)
        return ret;
    return security_kernel_post_read_file(file, buf, *size, id);
}


    #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h 
index 53f5ca41519a..f061ccb8d0b4 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -8,6 +8,7 @@
  #define __kernel_read_file_id(id) \
      id(UNKNOWN, unknown)        \
      id(FIRMWARE, firmware)        \
+    id(FIRMWARE_PARTIAL_READ, firmware)    \
      id(FIRMWARE_PREALLOC_BUFFER, firmware)    \
      id(FIRMWARE_EFI_EMBEDDED, firmware)    \

And again, sorry that this was in here as a misleading example.


      id(MODULE, kernel-module)        \
@@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id) 
      return kernel_read_file_str[id];
  }
  +int kernel_pread_file(struct file *file,
+              void **buf, loff_t *size, loff_t pos,
+              loff_t max_size,
+              enum kernel_read_file_id id);
  int kernel_read_file(struct file *file,
               void **buf, loff_t *size, loff_t max_size,
               enum kernel_read_file_id id);
+int kernel_pread_file_from_path(const char *path,
+                void **buf, loff_t *size, loff_t pos,
+                loff_t max_size,
+                enum kernel_read_file_id id);
  int kernel_read_file_from_path(const char *path,
                     void **buf, loff_t *size, loff_t max_size,
                     enum kernel_read_file_id id);
+int kernel_pread_file_from_path_initns(const char *path,
+                       void **buf, loff_t *size, loff_t pos,
+                       loff_t max_size,
+                       enum kernel_read_file_id id);
  int kernel_read_file_from_path_initns(const char *path,
                        void **buf, loff_t *size, loff_t max_size,
                        enum kernel_read_file_id id);
+int kernel_pread_file_from_fd(int fd,
+                  void **buf, loff_t *size, loff_t pos,
+                  loff_t max_size,
+                  enum kernel_read_file_id id);
  int kernel_read_file_from_fd(int fd,
                   void **buf, loff_t *size, loff_t max_size,
                   enum kernel_read_file_id id);

I remain concerned that adding these helpers will lead a poor
interaction with LSMs, but I guess I get to hold my tongue. :)
I only need kernel_pread_file and kernel_pread_file_from_path_initns.  kernel_pread_file_from_fd and kernel_pread_file_from_path were only added for completeness. And are really only helper functions called by their kernel_read_file* counterparts at this time.  So they can be removed from this patch if that helps? 
We could add pread functions that are "unsafe" in nature instead then?
As I certainly do not need any integrity checks on the file for my driver.  The real check is done by the card the data is loaded to whether is passes the linux security checks or not. And then, if someone does want to do something "safe" with preads another kernel_read_file_securelock/unlock could be added for those that need security for their partial reads?
[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux