Re: [PATCH 1/2] fs: __read: fix EOF count clamping for negative file sizes

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

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 <s.kerkmann@pengutronix.de>
>> ---
>>  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    |