On Tue, Jul 2, 2024 at 10:17 AM lei lu <llfamsec@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> ---
> 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..75d31bc11567 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;
There is a special scene here.
1) In label check_dirty_page_table, "while ((dp = enum_rstbl(dptbl,
dp)))" thinks
rsize is the size of DIR_PAGE_ENTRY_32 (esize1)
2) In label end_conv_1, "while ((dp = enum_rstbl(dptbl, dp)))" thinks
the same rsize
is the size of DIR_PAGE_ENTRY (esize2)
it seems that “esize2 + 4 = esize1”. So if rsize >= esize1, also rsize
>= esize2.
> +
> 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(*dp)))) {
The code should be modified to
"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
>
