| Message ID | 20240611182928.12813-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [xfsprogs] xfs_io: fix mread with length 1 mod page size | expand |
On Tue, Jun 11, 2024 at 11:29:28AM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Fix a weird bug in mread where if you passed it a length that was 1 > modulo the page size, for example > > xfs_io -r file -c "mmap -r 0 8192" -c "mread -v 0 4097" > > ... it never reset its pointer into the buffer into which it copies the > data from the memory map. This caused an out-of-bounds write, which > depending on the length passed could be very large and reliably > segfault. Also nothing was printed, despite the use of -v option. > > (I don't know if this case gets reached by any existing xfstest, but > presumably not. I noticed it while working on a patch to an xfstest.) > > Signed-off-by: Eric Biggers <ebiggers@google.com> Yeah, the current logic doesn't make much sense to me either. This new stuff reads much more straightforwardly. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > io/mmap.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/io/mmap.c b/io/mmap.c > index 85087f57..4c03e3d5 100644 > --- a/io/mmap.c > +++ b/io/mmap.c > @@ -469,38 +469,30 @@ mread_f( > dumplen = length % pagesize; > if (!dumplen) > dumplen = pagesize; > > if (rflag) { > - for (tmp = length - 1, c = 0; tmp >= 0; tmp--, c = 1) { > - *bp = *(((char *)mapping->addr) + dumpoffset + tmp); > - cnt++; > - if (c && cnt == dumplen) { > + for (tmp = length - 1; tmp >= 0; tmp--) { > + bp[cnt++] = ((char *)mapping->addr)[dumpoffset + tmp]; > + if (cnt == dumplen) { > if (dump) { > dump_buffer(printoffset, dumplen); > printoffset += dumplen; > } > - bp = (char *)io_buffer; > dumplen = pagesize; > cnt = 0; > - } else { > - bp++; > } > } > } else { > - for (tmp = 0, c = 0; tmp < length; tmp++, c = 1) { > - *bp = *(((char *)mapping->addr) + dumpoffset + tmp); > - cnt++; > - if (c && cnt == dumplen) { > + for (tmp = 0; tmp < length; tmp++) { > + bp[cnt++] = ((char *)mapping->addr)[dumpoffset + tmp]; > + if (cnt == dumplen) { > if (dump) > dump_buffer(printoffset + tmp - > (dumplen - 1), dumplen); > - bp = (char *)io_buffer; > dumplen = pagesize; > cnt = 0; > - } else { > - bp++; > } > } > } > return 0; > } > > base-commit: df4bd2d27189a98711fd35965c18bee25a25a9ea > -- > 2.45.1 > >
diff --git a/io/mmap.c b/io/mmap.c index 85087f57..4c03e3d5 100644 --- a/io/mmap.c +++ b/io/mmap.c @@ -469,38 +469,30 @@ mread_f( dumplen = length % pagesize; if (!dumplen) dumplen = pagesize; if (rflag) { - for (tmp = length - 1, c = 0; tmp >= 0; tmp--, c = 1) { - *bp = *(((char *)mapping->addr) + dumpoffset + tmp); - cnt++; - if (c && cnt == dumplen) { + for (tmp = length - 1; tmp >= 0; tmp--) { + bp[cnt++] = ((char *)mapping->addr)[dumpoffset + tmp]; + if (cnt == dumplen) { if (dump) { dump_buffer(printoffset, dumplen); printoffset += dumplen; } - bp = (char *)io_buffer; dumplen = pagesize; cnt = 0; - } else { - bp++; } } } else { - for (tmp = 0, c = 0; tmp < length; tmp++, c = 1) { - *bp = *(((char *)mapping->addr) + dumpoffset + tmp); - cnt++; - if (c && cnt == dumplen) { + for (tmp = 0; tmp < length; tmp++) { + bp[cnt++] = ((char *)mapping->addr)[dumpoffset + tmp]; + if (cnt == dumplen) { if (dump) dump_buffer(printoffset + tmp - (dumplen - 1), dumplen); - bp = (char *)io_buffer; dumplen = pagesize; cnt = 0; - } else { - bp++; } } } return 0; }