Hi Sascha, On 9/4/26 11:20, Sascha Hauer wrote: > Hi Stefan, > > On 2026-09-03 18:29, Stefan Kerkmann wrote: >> __read() clamps count to the bytes remaining until EOF, but compared >> the signed 64-bit f->f_pos/f->f_size (loff_t) against count (size_t), >> which is 32-bit on 32-bit arches and 64-bit on 64-bit arches. >> >> This made the comparison type targent dependent, breaking it for >> negative file sizes except for the FILE_SIZE_STREAM sentinel. >> >> - On 32-bit arches count is converted to the signed 64-bit type of >> f->f_pos, so for e.g. f->f_size = -512 the comparison f->f_pos + count >> > f->f_size evaluated true. The clamp then assigned the negative >> difference of f->f_size - f->f_pos to the unsigned count, wrapping it >> to a value near 2^32 and turning a small read into a huge out of >> bounds read request. >> >> - On 64-bit arches size_t cannot be represented by signed 64-bit, so the >> arithmetic C conversions turned the whole comparison unsigned: >> f->f_size = -512 was reinterpreted as a value near 2^64, the >> comparison stayed false and the clamp never ran, leaving count >> unclamped and the bogus size undetected. >> >> __read() now rejects negative file sizes (except for the >> FILE_SIZE_STREAM sentinel) with -EINVAL. Reads at or past the end of the >> file (reachable via pread() with an offset beyond EOF) now return 0. >> Remaining reads are clamped to the bytes left until EOF. >> >> Signed-off-by: Stefan Kerkmann <[email protected]> >> --- >> fs/fs.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fs.c b/fs/fs.c >> index dc6c30802d..a8f2b78294 100644 >> --- a/fs/fs.c >> +++ b/fs/fs.c >> @@ -427,8 +427,16 @@ static ssize_t __read(struct file *f, void *buf, size_t >> count) >> if (fsdrv != ramfs_driver) >> assert_command_context(); >> >> - if (f->f_size != FILE_SIZE_STREAM && f->f_pos + count > f->f_size) >> - count = f->f_size - f->f_pos; >> + if (f->f_size != FILE_SIZE_STREAM) { >> + if (f->f_size < 0) { >> + ret = -EINVAL; >> + goto out; >> + } > > I think we should start by rejecting negative file sizes at open time as > done in the patch I just sent. This is likely not the full solution to > the problem as f_pos + count could still become negative, but we > shouldn't allow to even open a file with negative file size. >
Agreed, that is cleaner and better. Depending on whether you incorporate the checks into your series I can send another series that builds on top of yours. > Sascha > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > Best regards, Stefan -- Pengutronix e.K. | Stefan Kerkmann | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-128 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
