Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <4266e5dd-39d8-05ec-d420-4f4c78c5f016@gmail.com>
Date:   Fri, 13 Oct 2023 15:19:50 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] jfs : fs array-index-out-of-bounds in txCommit
To:     Dave Kleikamp <dave.kleikamp@oracle.com>
Cc:     linux-kernel@vger.kernel.org, jfs-discussion@lists.sourceforge.net,
        Linux-kernel-mentees@lists.linuxfoundation.org,
        syzbot+0558d19c373e44da3c18@syzkaller.appspotmail.com
References: <20230919155542.4354-1-ghandatmanas@gmail.com>
 <423182d1-3d75-4a73-9011-da24658a40cc@oracle.com>
 <3d4e0f62-35bb-4b18-a64c-191c88415032@gmail.com>
 <52616ab2-6f8a-4313-a687-91a8d1081c8b@oracle.com>
Content-Language: en-US
From:   Manas Ghandat <ghandatmanas@gmail.com>
In-Reply-To: <52616ab2-6f8a-4313-a687-91a8d1081c8b@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@oracle.com>
> ---
>  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.