Currently there is no check for out of bound access for xad in the
struct xtpage_t. Added the required check at various places for the same
Signed-off-by: Manas Ghandat <[email protected]>
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=0558d19c373e44da3c18
Fixes: df0cc57e057f
---
fs/jfs/jfs_txnmgr.c | 4 ++++
fs/jfs/jfs_xtree.c | 6 ++++++
2 files changed, 10 insertions(+)
diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
index ce4b4760fcb1..6c6640942bed 100644
--- a/fs/jfs/jfs_txnmgr.c
+++ b/fs/jfs/jfs_txnmgr.c
@@ -1722,6 +1722,10 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
jfs_err("xtLog: lwm > next");
goto out;
}
+ if (lwm >= XTROOTMAXSLOT) {
+ jfs_err("xtLog: lwm out of range");
+ goto out;
+ }
tlck->flag |= tlckUPDATEMAP;
xadlock->flag = mlckALLOCXADLIST;
xadlock->count = next - lwm;
diff --git a/fs/jfs/jfs_xtree.c b/fs/jfs/jfs_xtree.c
index 2d304cee884c..57569c52663e 100644
--- a/fs/jfs/jfs_xtree.c
+++ b/fs/jfs/jfs_xtree.c
@@ -357,6 +357,9 @@ static int xtSearch(struct inode *ip, s64 xoff, s64 *nextp,
for (base = XTENTRYSTART; lim; lim >>= 1) {
index = base + (lim >> 1);
+ if (index >= XTROOTMAXSLOT)
+ goto out;
+
XT_CMP(cmp, xoff, &p->xad[index], t64);
if (cmp == 0) {
/*
@@ -618,6 +621,9 @@ int xtInsert(tid_t tid, /* transaction id */
memmove(&p->xad[index + 1], &p->xad[index],
(nextindex - index) * sizeof(xad_t));
+ if (index >= XTROOTMAXSLOT)
+ goto out;
+
/* insert the new entry: mark the entry NEW */
xad = &p->xad[index];
XT_PUTENTRY(xad, xflag, xoff, xlen, xaddr);
--
2.37.2
On 9/19/23 10:55AM, Manas Ghandat wrote:
> Currently there is no check for out of bound access for xad in the
> struct xtpage_t. Added the required check at various places for the same
This won't work for the same reason I mentioned here:
https://lore.kernel.org/lkml/[email protected]/
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
>
> Signed-off-by: Manas Ghandat <[email protected]>
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=0558d19c373e44da3c18
> Fixes: df0cc57e057f
> ---
> fs/jfs/jfs_txnmgr.c | 4 ++++
> fs/jfs/jfs_xtree.c | 6 ++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c
> index ce4b4760fcb1..6c6640942bed 100644
> --- a/fs/jfs/jfs_txnmgr.c
> +++ b/fs/jfs/jfs_txnmgr.c
> @@ -1722,6 +1722,10 @@ static void xtLog(struct jfs_log * log, struct tblock * tblk, struct lrd * lrd,
> jfs_err("xtLog: lwm > next");
> goto out;
> }
> + if (lwm >= XTROOTMAXSLOT) {
> + jfs_err("xtLog: lwm out of range");
> + goto out;
> + }
> tlck->flag |= tlckUPDATEMAP;
> xadlock->flag = mlckALLOCXADLIST;
> xadlock->count = next - lwm;
> diff --git a/fs/jfs/jfs_xtree.c b/fs/jfs/jfs_xtree.c
> index 2d304cee884c..57569c52663e 100644
> --- a/fs/jfs/jfs_xtree.c
> +++ b/fs/jfs/jfs_xtree.c
> @@ -357,6 +357,9 @@ static int xtSearch(struct inode *ip, s64 xoff, s64 *nextp,
> for (base = XTENTRYSTART; lim; lim >>= 1) {
> index = base + (lim >> 1);
>
> + if (index >= XTROOTMAXSLOT)
> + goto out;
> +
> XT_CMP(cmp, xoff, &p->xad[index], t64);
> if (cmp == 0) {
> /*
> @@ -618,6 +621,9 @@ int xtInsert(tid_t tid, /* transaction id */
> memmove(&p->xad[index + 1], &p->xad[index],
> (nextindex - index) * sizeof(xad_t));
>
> + if (index >= XTROOTMAXSLOT)
> + goto out;
> +
> /* insert the new entry: mark the entry NEW */
> xad = &p->xad[index];
> XT_PUTENTRY(xad, xflag, xoff, xlen, xaddr);
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.
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 <[email protected]>
---
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;
/*
--
2.42.0
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 <[email protected]>
> ---
> 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.
On 10/13/23 4:49AM, Manas Ghandat wrote:
>
> I tested this patch and it has not triggered any bug.
>
Great. Thanks for the help!
Shaggy