[syslinux] Missing Error Condition Check in core/fs/fs.c

Shao Miller Shao.Miller at yrdsb.edu.on.ca
Tue Aug 2 13:14:37 PDT 2011


On 8/2/2011 14:24, H. Peter Anvin wrote:
> On 08/02/2011 12:10 AM, Shao Miller wrote:
>> In 'core/fs/fs.c', 'fs.root' is set, but the 'iget_root' function
>> pointer call might return a null pointer and we don't check for it. A
>> symptom was that QEmu crashed as EIP landed outside of memory. - Shao
>>
>>
>> /* start out in the root directory */
>> if (fs.fs_ops->iget_root) {
>> fs.root = fs.fs_ops->iget_root(&fs);
>> /* Maybe we should check 'fs.root' here */
>> fs.cwd = get_inode(fs.root);
>> }
>>
>
> We're kind of dead if we can't get the root directory, no?
>
> I guess we should panic at that point...

Agreed.

Two patches, attached and below.

- Shao Miller

-----FIRST PATCH-----

 From c9e609b14db68bed620aa911ed4f3587702d2dd3 Mon Sep 17 00:00:00 2001
From: Shao Miller <shao.miller at yrdsb.edu.on.ca>
Date: Tue, 2 Aug 2011 15:58:50 -0400
Subject: [PATCH] core: Run Nindent on core/fs/fs.c

Signed-off-by: Shao Miller <shao.miller at yrdsb.edu.on.ca>
---
  core/fs/fs.c |   58 
++++++++++++++++++++++++++++------------------------------
  1 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/core/fs/fs.c b/core/fs/fs.c
index ad2fb37..95d747d 100644
--- a/core/fs/fs.c
+++ b/core/fs/fs.c
@@ -6,7 +6,7 @@
  #include "cache.h"

  /* The currently mounted filesystem */
-struct fs_info *this_fs = NULL;        /* Root filesystem */
+struct fs_info *this_fs = NULL;    /* Root filesystem */

  /* Actual file structures (we don't have malloc yet...) */
  struct file files[MAX_OPEN];
@@ -77,7 +77,7 @@ void _close_file(struct file *file)
   * Convert between a 16-bit file handle and a file structure
   */

-void pm_load_config(com32sys_t *regs)
+void pm_load_config(com32sys_t * regs)
  {
      int err;

@@ -89,10 +89,10 @@ void pm_load_config(com32sys_t *regs)
      set_flags(regs, err ? EFLAGS_ZF : 0);
  }

-void pm_mangle_name(com32sys_t *regs)
+void pm_mangle_name(com32sys_t * regs)
  {
      const char *src = MK_PTR(regs->ds, regs->esi.w[0]);
-    char       *dst = MK_PTR(regs->es, regs->edi.w[0]);
+    char *dst = MK_PTR(regs->es, regs->edi.w[0]);

      mangle_name(dst, src);
  }
@@ -102,7 +102,7 @@ void mangle_name(char *dst, const char *src)
      this_fs->fs_ops->mangle_name(dst, src);
  }

-void getfssec(com32sys_t *regs)
+void getfssec(com32sys_t * regs)
  {
      int sectors;
      bool have_more;
@@ -125,13 +125,13 @@ void getfssec(com32sys_t *regs)
       */
      if (!have_more) {
      _close_file(file);
-        regs->esi.w[0] = 0;
+    regs->esi.w[0] = 0;
      }

      regs->ecx.l = bytes_read;
  }

-void getfsbytes(com32sys_t *regs)
+void getfsbytes(com32sys_t * regs)
  {
      int sectors;
      bool have_more;
@@ -154,13 +154,13 @@ void getfsbytes(com32sys_t *regs)
       */
      if (!have_more) {
      _close_file(file);
-        regs->esi.w[0] = 0;
+    regs->esi.w[0] = 0;
      }

      regs->ecx.l = bytes_read;
  }

-size_t pmapi_read_file(uint16_t *handle, void *buf, size_t sectors)
+size_t pmapi_read_file(uint16_t * handle, void *buf, size_t sectors)
  {
      bool have_more;
      size_t bytes_read;
@@ -181,19 +181,19 @@ size_t pmapi_read_file(uint16_t *handle, void 
*buf, size_t sectors)
      return bytes_read;
  }

-void pm_searchdir(com32sys_t *regs)
+void pm_searchdir(com32sys_t * regs)
  {
      char *name = MK_PTR(regs->ds, regs->edi.w[0]);
      int rv;

      rv = searchdir(name);
      if (rv < 0) {
-    regs->esi.w[0]  = 0;
-    regs->eax.l     = 0;
+    regs->esi.w[0] = 0;
+    regs->eax.l = 0;
      regs->eflags.l |= EFLAGS_ZF;
      } else {
-    regs->esi.w[0]  = rv;
-    regs->eax.l     = handle_to_file(rv)->inode->size;
+    regs->esi.w[0] = rv;
+    regs->eax.l = handle_to_file(rv)->inode->size;
      regs->eflags.l &= ~EFLAGS_ZF;
      }
  }
@@ -229,7 +229,7 @@ int searchdir(const char *name)
      goto err;

      do {
-    got_link:
+got_link:
      if (*p == '/') {
          put_inode(parent);
          parent = get_inode(this_fs->root);
@@ -272,8 +272,7 @@ int searchdir(const char *name)
              int total_len = inode->size + name_len + 2;
              int link_len;

-            if (!this_fs->fs_ops->readlink ||
-            --symlink_count == 0       ||      /* limit check */
+            if (!this_fs->fs_ops->readlink || --symlink_count == 0 
||    /* limit check */
              total_len > MAX_SYMLINK_BUF)
              goto err;

@@ -293,7 +292,7 @@ int searchdir(const char *name)
              if (link_len > 0 && q[-1] != '/')
                  *q++ = '/';

-            memcpy(q, p, name_len+1);
+            memcpy(q, p, name_len + 1);
              } else {
              *q = '\0';
              }
@@ -328,7 +327,7 @@ int searchdir(const char *name)
      if (!inode)
      goto err;

-    file->inode  = inode;
+    file->inode = inode;
      file->offset = 0;

      return file_to_handle(file);
@@ -362,14 +361,14 @@ int open_file(const char *name, struct 
com32_filedata *filedata)
      return -1;
      }

-    filedata->size    = file->inode->size;
-    filedata->blocklg2    = SECTOR_SHIFT(file->fs);
-    filedata->handle    = rv;
+    filedata->size = file->inode->size;
+    filedata->blocklg2 = SECTOR_SHIFT(file->fs);
+    filedata->handle = rv;

      return rv;
  }

-void pm_open_file(com32sys_t *regs)
+void pm_open_file(com32sys_t * regs)
  {
      int rv;
      struct file *file;
@@ -399,7 +398,7 @@ void close_file(uint16_t handle)
      }
  }

-void pm_close_file(com32sys_t *regs)
+void pm_close_file(com32sys_t * regs)
  {
      close_file(regs->esi.w[0]);
  }
@@ -415,12 +414,12 @@ void pm_close_file(com32sys_t *regs)
   */
  __bss16 uint16_t SectorSize, SectorShift;

-void fs_init(com32sys_t *regs)
+void fs_init(com32sys_t * regs)
  {
      static struct fs_info fs;    /* The actual filesystem buffer */
      uint8_t disk_devno = regs->edx.b[0];
      uint8_t disk_cdrom = regs->edx.b[1];
-    sector_t disk_offset = regs->ecx.l | ((sector_t)regs->ebx.l << 32);
+    sector_t disk_offset = regs->ecx.l | ((sector_t) regs->ebx.l << 32);
      uint16_t disk_heads = regs->esi.w[0];
      uint16_t disk_sectors = regs->edi.w[0];
      uint32_t maxtransfer = regs->ebp.l;
@@ -457,14 +456,13 @@ void fs_init(com32sys_t *regs)
      }
      if (blk_shift < 0) {
      printf("No valid file system found!\n");
-    while (1)
-        ;
+    while (1) ;
      }
      this_fs = &fs;

      /* initialize the cache */
      if (fs.fs_dev && fs.fs_dev->cache_data)
-        cache_init(fs.fs_dev, blk_shift);
+    cache_init(fs.fs_dev, blk_shift);

      /* start out in the root directory */
      if (fs.fs_ops->iget_root) {
@@ -473,5 +471,5 @@ void fs_init(com32sys_t *regs)
      }

      SectorShift = fs.sector_shift;
-    SectorSize  = fs.sector_size;
+    SectorSize = fs.sector_size;
  }
-- 
1.6.0.4

-----SECOND PATCH-----

 From b17a8e9992c99135daecc6edf8816a947b71a557 Mon Sep 17 00:00:00 2001
From: Shao Miller <shao.miller at yrdsb.edu.on.ca>
Date: Tue, 2 Aug 2011 16:07:50 -0400
Subject: [PATCH] core: Use kaboom in a couple of places

If we do not find a valid filesystem and if we do not find
a root directory, use kaboom() to terminate.

Signed-off-by: Shao Miller <shao.miller at yrdsb.edu.on.ca>
---
  core/fs/fs.c |    6 +++++-
  1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/core/fs/fs.c b/core/fs/fs.c
index 95d747d..9a97525 100644
--- a/core/fs/fs.c
+++ b/core/fs/fs.c
@@ -456,7 +456,7 @@ void fs_init(com32sys_t * regs)
      }
      if (blk_shift < 0) {
      printf("No valid file system found!\n");
-    while (1) ;
+    kaboom();
      }
      this_fs = &fs;

@@ -467,6 +467,10 @@ void fs_init(com32sys_t * regs)
      /* start out in the root directory */
      if (fs.fs_ops->iget_root) {
      fs.root = fs.fs_ops->iget_root(&fs);
+    if (!fs.root) {
+        printf("No root directory found!\n");
+        kaboom();
+    }
      fs.cwd = get_inode(fs.root);
      }

-- 
1.6.0.4


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-core-Run-Nindent-on-core-fs-fs.c.patch
URL: <http://www.zytor.com/pipermail/syslinux/attachments/20110802/2fa0023e/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-core-Use-kaboom-in-a-couple-of-places.patch
URL: <http://www.zytor.com/pipermail/syslinux/attachments/20110802/2fa0023e/attachment-0001.ksh>


More information about the Syslinux mailing list