Re: [PATCH] jfs : fs array-index-out-of-bounds in txCommit

From: Manas Ghandat
Date: Fri Oct 13 2023 - 05:50:00 EST


On 05/10/23 19:50, Dave Kleikamp wrote:

On 10/5/23 12:15AM, Manas Ghandat wrote:
On 04/10/23 00:46, Dave Kleikamp wrote:
The size of the xad array can be either XTROOTMAXSLOT or XTPAGEMAXSLOT depending on whether it is the root, imbedded in the inode, or not. It is currently declared with the smaller value so we can use xtpage_t within the inode.

I had promised to address this, but haven't gotten to it yet. I'll try to carve out some time to do that.

Thanks,
Shaggy

Can you guide with the workflow of how things should be done. I can try to work on it and resolve the issue.


I was able to cobble this together. It compiles cleanly, but I haven't
tested it yet.

In order to make array bounds checking sane, provide a separate
definition of the in-inode xtree root and the external xtree page.

Signed-off-by: Dave Kleikamp <dave.kleikamp@xxxxxxxxxx>
---
 fs/jfs/jfs_dinode.h |  2 +-
 fs/jfs/jfs_imap.c   |  6 +++---
 fs/jfs/jfs_incore.h |  2 +-
 fs/jfs/jfs_txnmgr.c |  4 ++--
 fs/jfs/jfs_xtree.c  |  4 ++--
 fs/jfs/jfs_xtree.h  | 37 +++++++++++++++++++++++--------------
 6 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/fs/jfs/jfs_dinode.h b/fs/jfs/jfs_dinode.h
index 6b231d0d0071..603aae17a693 100644
--- a/fs/jfs/jfs_dinode.h
+++ b/fs/jfs/jfs_dinode.h
@@ -96,7 +96,7 @@ struct dinode {
 #define di_gengen    u._file._u1._imap._gengen

             union {
-                xtpage_t _xtroot;
+                xtroot_t _xtroot;
                 struct {
                     u8 unused[16];    /* 16: */
                     dxd_t _dxd;    /* 16: */
diff --git a/fs/jfs/jfs_imap.c b/fs/jfs/jfs_imap.c
index 1b267eec3f36..394e0af0e5df 100644
--- a/fs/jfs/jfs_imap.c
+++ b/fs/jfs/jfs_imap.c
@@ -670,7 +670,7 @@ int diWrite(tid_t tid, struct inode *ip)
          * This is the special xtree inside the directory for storing
          * the directory table
          */
-        xtpage_t *p, *xp;
+        xtroot_t *p, *xp;
         xad_t *xad;

         jfs_ip->xtlid = 0;
@@ -684,7 +684,7 @@ int diWrite(tid_t tid, struct inode *ip)
          * copy xtree root from inode to dinode:
          */
         p = &jfs_ip->i_xtroot;
-        xp = (xtpage_t *) &dp->di_dirtable;
+        xp = (xtroot_t *) &dp->di_dirtable;
         lv = ilinelock->lv;
         for (n = 0; n < ilinelock->index; n++, lv++) {
             memcpy(&xp->xad[lv->offset], &p->xad[lv->offset],
@@ -713,7 +713,7 @@ int diWrite(tid_t tid, struct inode *ip)
      *    regular file: 16 byte (XAD slot) granularity
      */
     if (type & tlckXTREE) {
-        xtpage_t *p, *xp;
+        xtroot_t *p, *xp;
         xad_t *xad;

         /*
diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h
index 721def69e732..dd4264aa9bed 100644
--- a/fs/jfs/jfs_incore.h
+++ b/fs/jfs/jfs_incore.h
@@ -66,7 +66,7 @@ struct jfs_inode_info {
     lid_t    xtlid;        /* lid of xtree lock on directory */
     union {
         struct {
-            xtpage_t _xtroot;    /* 288: xtree root */
+            xtroot_t _xtroot;    /* 288: xtree root */
             struct inomap *_imap;    /* 4: inode map header    */
         } file;
         struct {
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index ce4b4760fcb1..dccc8b3f1045 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -783,7 +783,7 @@ struct tlock *txLock(tid_t tid, struct inode *ip, struct metapage * mp,
             if (mp->xflag & COMMIT_PAGE)
                 p = (xtpage_t *) mp->data;
             else
-                p = &jfs_ip->i_xtroot;
+                p = (xtpage_t *) &jfs_ip->i_xtroot;
             xtlck->lwm.offset =
                 le16_to_cpu(p->header.nextindex);
         }
@@ -1676,7 +1676,7 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,

     if (tlck->type & tlckBTROOT) {
         lrd->log.redopage.type |= cpu_to_le16(LOG_BTROOT);
-        p = &JFS_IP(ip)->i_xtroot;
+        p = (xtpage_t *) &JFS_IP(ip)->i_xtroot;
         if (S_ISDIR(ip->i_mode))
             lrd->log.redopage.type |=
                 cpu_to_le16(LOG_DIR_XTREE);
diff --git a/fs/jfs/jfs_xtree.c b/fs/jfs/jfs_xtree.c
index 2d304cee884c..5ee618d17e77 100644
--- a/fs/jfs/jfs_xtree.c
+++ b/fs/jfs/jfs_xtree.c
@@ -1213,7 +1213,7 @@ xtSplitRoot(tid_t tid,
     struct xtlock *xtlck;
     int rc;

-    sp = &JFS_IP(ip)->i_xtroot;
+    sp = (xtpage_t *) &JFS_IP(ip)->i_xtroot;

     INCREMENT(xtStat.split);

@@ -2098,7 +2098,7 @@ int xtAppend(tid_t tid,        /* transaction id */
  */
 void xtInitRoot(tid_t tid, struct inode *ip)
 {
-    xtpage_t *p;
+    xtroot_t *p;

     /*
      * acquire a transaction lock on the root
diff --git a/fs/jfs/jfs_xtree.h b/fs/jfs/jfs_xtree.h
index ad7592191d76..0f6cf5a1ce75 100644
--- a/fs/jfs/jfs_xtree.h
+++ b/fs/jfs/jfs_xtree.h
@@ -65,24 +65,33 @@ struct xadlist {
 #define XTPAGEMAXSLOT    256
 #define XTENTRYSTART    2

-/*
- *    xtree page:
- */
-typedef union {
-    struct xtheader {
-        __le64 next;    /* 8: */
-        __le64 prev;    /* 8: */
+struct xtheader {
+    __le64 next;    /* 8: */
+    __le64 prev;    /* 8: */

-        u8 flag;    /* 1: */
-        u8 rsrvd1;    /* 1: */
-        __le16 nextindex;    /* 2: next index = number of entries */
-        __le16 maxentry;    /* 2: max number of entries */
-        __le16 rsrvd2;    /* 2: */
+    u8 flag;    /* 1: */
+    u8 rsrvd1;    /* 1: */
+    __le16 nextindex;    /* 2: next index = number of entries */
+    __le16 maxentry;    /* 2: max number of entries */
+    __le16 rsrvd2;    /* 2: */

-        pxd_t self;    /* 8: self */
-    } header;        /* (32) */
+    pxd_t self;    /* 8: self */
+};

+/*
+ *    xtree root (in inode):
+ */
+typedef union {
+    struct xtheader header;
     xad_t xad[XTROOTMAXSLOT];    /* 16 * maxentry: xad array */
+} xtroot_t;
+
+/*
+ *    xtree page:
+ */
+typedef union {
+    struct xtheader header;
+    xad_t xad[XTPAGEMAXSLOT];    /* 16 * maxentry: xad array */
 } xtpage_t;

 /*

I tested this patch and it has not triggered any bug.