mirror of
https://github.com/torvalds/linux.git
synced 2026-11-07 06:17:02 +01:00
fs/ntfs3: Fix heap overflow after ALIGN in mi_pack_runs()
When run_pack() returns a non-aligned byte count, ALIGN(err, 8)
inflates it by 1-7 bytes, and since the destination of the memmove() is
computed from that inflated value, the entire attribute tail could be
copied 1-7 bytes past where the record buffer ends.
The memmove() destination is next + new_run_size - run_size. For it to
stay within the record, new_run_size must not exceed run_size + dsize,
the space actually available.
Since new_run_size = ALIGN(err, 8), a fix could be to ensures there is
enough room that whatever ALIGN does to err, the result still fits within
the available space. Restricting the budget of run_pack() should
guarantee that.
Something like:
u32 avail = (run_size + dsize) & ~7u;
err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail, &plen);
Now, run_pack() should never return 0. As now, the function returns at
least 1. So this is fine. Negative values are caught as errors by
if(err<0).
The rounded-down avail may be smaller than the existing run size,
if avail=0 run_pack is called and will write run_buf[0] = 0; that will be
OOB, because the buffer size is zero, so imo the caller should avoid
calling run_pack() with avail=0.
After run_pack(), if plen = 0, then svcn + plen - 1 becomes svcn - 1.
So, after run_pack(), it may be worth ensuring if(!plen).
Lastly, I'm not sure if -ENOSPC is the right error code here, but
it seems reasonable for a space condition.
This heap out-of-bounds write requires a crafted filesystem image,
which is not in the kernel threat model, but fixing memory errors would be
nice to keep things secure.
Signed-off-by: Alexandro Calo <alexandro.calo@nozominetworks.com>
Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
This commit is contained in:
parent
eff11b5a1e
commit
5a5aef1142
|
|
@ -665,18 +665,32 @@ int mi_pack_runs(struct mft_inode *mi, struct ATTRIB *attr,
|
|||
u32 run_size = asize - run_off;
|
||||
u32 tail = used - aoff - asize;
|
||||
u32 dsize = sbi->record_size - used;
|
||||
u32 avail;
|
||||
|
||||
/* Make a maximum gap in current record. */
|
||||
memmove(next + dsize, next, tail);
|
||||
|
||||
/* Leave room for the 8-byte ALIGN() to avoid OOB write */
|
||||
avail = (run_size + dsize) & ~7u;
|
||||
|
||||
if (!avail) {
|
||||
memmove(next, next + dsize, tail);
|
||||
return -ENOSPC;
|
||||
}
|
||||
|
||||
/* Pack as much as possible. */
|
||||
err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), run_size + dsize,
|
||||
err = run_pack(run, svcn, len, Add2Ptr(attr, run_off), avail,
|
||||
&plen);
|
||||
if (err < 0) {
|
||||
memmove(next, next + dsize, tail);
|
||||
return err;
|
||||
}
|
||||
|
||||
if (!plen) {
|
||||
memmove(next, next + dsize, tail);
|
||||
return -ENOSPC;
|
||||
}
|
||||
|
||||
new_run_size = ALIGN(err, 8);
|
||||
|
||||
memmove(next + new_run_size - run_size, next + dsize, tail);
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user