* [PATCH v2] ntfs3: Add bounds checking to enum_rstbl()
@ 2024-07-09 3:48 lei lu
0 siblings, 0 replies; 3+ messages in thread
From: lei lu @ 2024-07-09 3:48 UTC (permalink / raw)
To: almaz.alexandrovich, ntfs3
Added bounds checking to make sure rsize is not less than the entry
size and make sure the remaining bytes are large enough to hold an
entry before accessing in the caller.
Signed-off-by: lei lu <llfamsec@gmail.com>
---
fs/ntfs3/fslog.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
index 855519713bf7..5bdb31ffab3a 100644
--- a/fs/ntfs3/fslog.c
+++ b/fs/ntfs3/fslog.c
@@ -609,12 +609,15 @@ static inline void add_client(struct CLIENT_REC *ca, u16 index, __le16 *head)
*head = cpu_to_le16(index);
}
-static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c)
+static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c, u16 esize)
{
__le32 *e;
u32 bprt;
u16 rsize = t ? le16_to_cpu(t->size) : 0;
+ if (rsize < esize)
+ return NULL;
+
if (!c) {
if (!t || !t->total)
return NULL;
@@ -626,6 +629,8 @@ static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c)
/* Loop until we hit the first one allocated, or the end of the list. */
for (bprt = bytes_per_rt(t); PtrOffset(t, e) < bprt;
e = Add2Ptr(e, rsize)) {
+ if (PtrOffset(t, Add2Ptr(e, esize)) > bprt)
+ return NULL;
if (*e == RESTART_ENTRY_ALLOCATED_LE)
return e;
}
@@ -641,7 +646,7 @@ static inline struct DIR_PAGE_ENTRY *find_dp(struct RESTART_TABLE *dptbl,
__le32 ta = cpu_to_le32(target_attr);
struct DIR_PAGE_ENTRY *dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
u64 dp_vcn = le64_to_cpu(dp->vcn);
if (dp->target_attr == ta && vcn >= dp_vcn &&
@@ -2950,7 +2955,7 @@ static struct OpenAttr *find_loaded_attr(struct ntfs_log *log,
{
struct OPEN_ATTR_ENRTY *oe = NULL;
- while ((oe = enum_rstbl(log->open_attr_tbl, oe))) {
+ while ((oe = enum_rstbl(log->open_attr_tbl, oe, sizeof(*oe)))) {
struct OpenAttr *op_attr;
if (ino_get(&oe->ref) != rno)
@@ -4182,7 +4187,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
goto end_conv_1;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(struct DIR_PAGE_ENTRY_32)))) {
struct DIR_PAGE_ENTRY_32 *dp0 = (struct DIR_PAGE_ENTRY_32 *)dp;
// NOTE: Danger. Check for of boundary.
memmove(&dp->vcn, &dp0->vcn_low,
@@ -4202,10 +4207,10 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
goto trace_dp_table;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
struct DIR_PAGE_ENTRY *next = dp;
- while ((next = enum_rstbl(dptbl, next))) {
+ while ((next = enum_rstbl(dptbl, next, sizeof(*next)))) {
if (next->target_attr == dp->target_attr &&
next->vcn == dp->vcn) {
if (le64_to_cpu(next->oldest_lsn) <
@@ -4290,7 +4295,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
/* Clear all of the Attr pointers. */
oe = NULL;
- while ((oe = enum_rstbl(oatbl, oe))) {
+ while ((oe = enum_rstbl(oatbl, oe, sizeof(*oe)))) {
if (!rst->major_ver) {
struct OPEN_ATTR_ENRTY_32 oe0;
@@ -4507,7 +4512,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
u64 lcn_e = lcn0 + le64_to_cpu(r->len) - 1;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
u32 j;
t32 = le32_to_cpu(dp->lcns_follow);
@@ -4640,14 +4645,14 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* the lowest lsn, and return it as the Redo lsn.
*/
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
t64 = le64_to_cpu(dp->oldest_lsn);
if (t64 && t64 < rlsn)
rlsn = t64;
}
tr = NULL;
- while ((tr = enum_rstbl(trtbl, tr))) {
+ while ((tr = enum_rstbl(trtbl, tr, sizeof(*tr)))) {
t64 = le64_to_cpu(tr->first_lsn);
if (t64 && t64 < rlsn)
rlsn = t64;
@@ -4668,7 +4673,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
oe = NULL;
next_open_attribute:
- oe = enum_rstbl(oatbl, oe);
+ oe = enum_rstbl(oatbl, oe, sizeof(*oe));
if (!oe) {
err = 0;
dp = NULL;
@@ -4770,7 +4775,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* Mapping that we have, and insert it into the appropriate run.
*/
next_dirty_page:
- dp = enum_rstbl(dptbl, dp);
+ dp = enum_rstbl(dptbl, dp, sizeof(*dp));
if (!dp)
goto do_redo_1;
@@ -4968,7 +4973,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
/* Scan Transaction Table. */
tr = NULL;
transaction_table_next:
- tr = enum_rstbl(trtbl, tr);
+ tr = enum_rstbl(trtbl, tr, sizeof(*tr));
if (!tr)
goto undo_action_done;
@@ -5142,7 +5147,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* the open attributes.
*/
oe = NULL;
- while ((oe = enum_rstbl(oatbl, oe))) {
+ while ((oe = enum_rstbl(oatbl, oe, sizeof(*oe)))) {
rno = ino_get(&oe->ref);
if (oe->is_attr_name == 1) {
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2] ntfs3: Add bounds checking to enum_rstbl()
@ 2024-09-18 17:17 lei lu
2024-09-18 17:43 ` lei lu
0 siblings, 1 reply; 3+ messages in thread
From: lei lu @ 2024-09-18 17:17 UTC (permalink / raw)
To: almaz.alexandrovich, ntfs3
Added bounds checking to make sure rsize is not less than the entry
size and make sure the remaining bytes are large enough to hold an
entry before accessing in the caller.
Signed-off-by: lei lu <llfamsec@gmail.com>
---
fs/ntfs3/fslog.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
index 855519713bf7..8a70af93e2ce 100644
--- a/fs/ntfs3/fslog.c
+++ b/fs/ntfs3/fslog.c
@@ -609,12 +609,15 @@ static inline void add_client(struct CLIENT_REC *ca, u16 index, __le16 *head)
*head = cpu_to_le16(index);
}
-static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c)
+static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c, u16 esize)
{
__le32 *e;
u32 bprt;
u16 rsize = t ? le16_to_cpu(t->size) : 0;
+ if (rsize < esize)
+ return NULL;
+
if (!c) {
if (!t || !t->total)
return NULL;
@@ -626,6 +629,8 @@ static inline void *enum_rstbl(struct RESTART_TABLE *t, void *c)
/* Loop until we hit the first one allocated, or the end of the list. */
for (bprt = bytes_per_rt(t); PtrOffset(t, e) < bprt;
e = Add2Ptr(e, rsize)) {
+ if (PtrOffset(t, Add2Ptr(e, esize)) > bprt)
+ return NULL;
if (*e == RESTART_ENTRY_ALLOCATED_LE)
return e;
}
@@ -641,7 +646,7 @@ static inline struct DIR_PAGE_ENTRY *find_dp(struct RESTART_TABLE *dptbl,
__le32 ta = cpu_to_le32(target_attr);
struct DIR_PAGE_ENTRY *dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
u64 dp_vcn = le64_to_cpu(dp->vcn);
if (dp->target_attr == ta && vcn >= dp_vcn &&
@@ -2950,7 +2955,7 @@ static struct OpenAttr *find_loaded_attr(struct ntfs_log *log,
{
struct OPEN_ATTR_ENRTY *oe = NULL;
- while ((oe = enum_rstbl(log->open_attr_tbl, oe))) {
+ while ((oe = enum_rstbl(log->open_attr_tbl, oe, sizeof(*oe)))) {
struct OpenAttr *op_attr;
if (ino_get(&oe->ref) != rno)
@@ -4182,7 +4187,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
goto end_conv_1;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(struct DIR_PAGE_ENTRY_32)))) {
struct DIR_PAGE_ENTRY_32 *dp0 = (struct DIR_PAGE_ENTRY_32 *)dp;
// NOTE: Danger. Check for of boundary.
memmove(&dp->vcn, &dp0->vcn_low,
@@ -4202,10 +4207,10 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
goto trace_dp_table;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
struct DIR_PAGE_ENTRY *next = dp;
- while ((next = enum_rstbl(dptbl, next))) {
+ while ((next = enum_rstbl(dptbl, next, sizeof(*next)))) {
if (next->target_attr == dp->target_attr &&
next->vcn == dp->vcn) {
if (le64_to_cpu(next->oldest_lsn) <
@@ -4290,11 +4295,11 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
/* Clear all of the Attr pointers. */
oe = NULL;
- while ((oe = enum_rstbl(oatbl, oe))) {
- if (!rst->major_ver) {
+ if (!rst->major_ver) {
+ while ((oe = enum_rstbl(oatbl, oe, sizeof(struct OPEN_ATTR_ENRTY_32)))) {
struct OPEN_ATTR_ENRTY_32 oe0;
- /* Really 'oe' points to OPEN_ATTR_ENRTY_32. */
+ /* Really 'oe' points to OPEN_ATTR_ENTRY_32. */
memcpy(&oe0, oe, SIZEOF_OPENATTRIBUTEENTRY0);
oe->bytes_per_index = oe0.bytes_per_index;
@@ -4304,7 +4309,8 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
oe->ref = oe0.ref;
oe->open_record_lsn = oe0.open_record_lsn;
}
-
+ }
+ while ((oe = enum_rstbl(oatbl, oe, sizeof(*oe)))) {
oe->is_attr_name = 0;
oe->ptr = NULL;
}
@@ -4507,7 +4513,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
u64 lcn_e = lcn0 + le64_to_cpu(r->len) - 1;
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
u32 j;
t32 = le32_to_cpu(dp->lcns_follow);
@@ -4640,14 +4646,14 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* the lowest lsn, and return it as the Redo lsn.
*/
dp = NULL;
- while ((dp = enum_rstbl(dptbl, dp))) {
+ while ((dp = enum_rstbl(dptbl, dp, sizeof(*dp)))) {
t64 = le64_to_cpu(dp->oldest_lsn);
if (t64 && t64 < rlsn)
rlsn = t64;
}
tr = NULL;
- while ((tr = enum_rstbl(trtbl, tr))) {
+ while ((tr = enum_rstbl(trtbl, tr, sizeof(*tr)))) {
t64 = le64_to_cpu(tr->first_lsn);
if (t64 && t64 < rlsn)
rlsn = t64;
@@ -4668,7 +4674,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
oe = NULL;
next_open_attribute:
- oe = enum_rstbl(oatbl, oe);
+ oe = enum_rstbl(oatbl, oe, sizeof(*oe));
if (!oe) {
err = 0;
dp = NULL;
@@ -4770,7 +4776,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* Mapping that we have, and insert it into the appropriate run.
*/
next_dirty_page:
- dp = enum_rstbl(dptbl, dp);
+ dp = enum_rstbl(dptbl, dp, sizeof(*dp));
if (!dp)
goto do_redo_1;
@@ -4968,7 +4974,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
/* Scan Transaction Table. */
tr = NULL;
transaction_table_next:
- tr = enum_rstbl(trtbl, tr);
+ tr = enum_rstbl(trtbl, tr, sizeof(*tr));
if (!tr)
goto undo_action_done;
@@ -5142,7 +5148,7 @@ int log_replay(struct ntfs_inode *ni, bool *initialized)
* the open attributes.
*/
oe = NULL;
- while ((oe = enum_rstbl(oatbl, oe))) {
+ while ((oe = enum_rstbl(oatbl, oe, sizeof(*oe)))) {
rno = ino_get(&oe->ref);
if (oe->is_attr_name == 1) {
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ntfs3: Add bounds checking to enum_rstbl()
2024-09-18 17:17 [PATCH v2] ntfs3: Add bounds checking to enum_rstbl() lei lu
@ 2024-09-18 17:43 ` lei lu
0 siblings, 0 replies; 3+ messages in thread
From: lei lu @ 2024-09-18 17:43 UTC (permalink / raw)
To: llfamsec; +Cc: almaz.alexandrovich, ntfs3
Hi, Konstantin,
In log_replay, enum_rstbl didn't check the real size of the entry. It
traverses based on RESTART_TABLE.size. There is a lack of boundary check
if the real size is not equal to RESTART_TABLE.size.
Here is a PoC to reproduce.
PoC:
1) Found NTFS_RESTART by CLIENT_REC[0].restart_lsn
NTFS_RESTART.open_attr_table_lsn: LSN where PoC is located
NTFS_RESTART.open_attr_len: 0x1
2) Layout PoC as follow in a LSN area as follow:
LFS_RECORD_HDR.this_lsn: 0x28455b
LFS_RECORD_HDR.client_prev_lsn: 0x0
LFS_REOCRD_HDR.client_undo_next_lsn: 0x0
LFS_RECORD_HDR.client_data_len: 0x98
LFS_RECORD_HDR.CLIENT_ID.seq_num: 0x0
LFS_RECORD_HDR.CLIENT_ID.client_idx: 0x0
LFS_RECORD_HDR.record_type: 0x1
LFS_RECORD_HDR.transact_id: 0x18
LFS_RECORD_HDR.flags: 0x4
LOG_REC_HDR.redo_op: 0x0
LOG_REC_HDR.undo_op: 0x0
LOG_REC_HDR.redo_off: 0x98-sizeof(RESTART_TABLE)-RESTART_TABLE.size (redo_off&7 != 0)
LOG_REC_HDR.redo_len: 0x0
LOG_REC_HDR.undo_off: 0x0
LOG_REC_HDR.undo_len: 0x0
LOG_REC_HDR.target_attr: 0x0
LOG_REC_HDR.lcns_follow: 0x0
LOG_REC_HDR.record_off: 0x0
LOG_REC_HDR.attr_off: 0x0
LOG_REC_HDR.cluster_off: 0x0
LOG_REC_HDR.reserved: 0x0
LOG_REC_HDR.target_vcn: 0x0
LOG_REC_HDR.page_lcns[0]: 0x0
RESTART_TABLE.size: 8
RESTART_TABLE.used: 0x1
RESTART_TABLE.total: 0x1
RESTART_TABLE.res[0]: 0x0
RESTART_TABLE.res[1]: 0x0
RESTART_TABLE.res[2]: 0x0
RESTART_TABLE.free_goal: 0x0
RESTART_TABLE.first_free: 0x0
RESTART_TABLE.last_free: 0x0
entry[0][0:4]: 0xFFFFFFFF (OPEN_ATTR_ENRTY[0].next)
OPEN_ATTR_ENRTY[0].bytes_per_index: 0x0
In the patch, taking DIR_PAGE_ENTRY_32/DIR_PAGE_ENTRY as an example, there are two
situations:
[1] https://elixir.bootlin.com/linux/v6.10-rc4/source/fs/ntfs3/fslog.c#L4186
If entry is DIR_PAGE_ENTRY_32, it seems to explain it as DIR_PAGE_ENTRY, but the
entry size is still DIR_PAGE_ENTRY_32, so we use sizeof(struct DIR_PAGE_ENTRY_32)
[2] https://elixir.bootlin.com/linux/v6.10-rc4/source/fs/ntfs3/fslog.c#L4206
- If entry is DIR_PAGE_ENTRY directly, we use sizeof(*dp)
- If entry is DIR_PAGE_ENTRY_32, but it is explained as DIR_PAGE_ENTRY. Because
We check it in [1], and sizeof(struct DIR_PAGE_ENTRY) is smaller, so we also
use sizeof(*dp)
Thanks,
LL
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-09-18 17:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-18 17:17 [PATCH v2] ntfs3: Add bounds checking to enum_rstbl() lei lu
2024-09-18 17:43 ` lei lu
-- strict thread matches above, loose matches on Subject: below --
2024-07-09 3:48 lei lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox