On Tue, Jul 9, 2024 at 11:48 AM lei lu <llfamsec@xxxxxxxxx> wrote:
>
> 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)))) {
Same problem here. There are two possible values for entry size:
1) SIZEOF_OPENATTRIBUTEENTRY0
2) sizeof(struct OPEN_ATTR_ENRTY)
Hi, Konstantin,
Could you please take some time to review this patch?
Thanks,
LL
> > 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
> >
