Re: [RFC PATCH v2] ceph: ceph: fix out-of-bound array access when doing a file read

[Luis Henriques <luis.henriques@linux.dev>]

Hi!

On Thu, Nov 28 2024, Alex Markuze wrote:
> On Thu, Nov 28, 2024 at 7:43 PM Luis Henriques <luis.henriques@linux.dev> wrote:
>>
>> Hi Alex,
>>
>> [ Thank you for looking into this. ]
>>
>> On Wed, Nov 27 2024, Alex Markuze wrote:
>>
>> > Hi, Folks.
>> > AFAIK there is no side effect that can affect MDS with this fix.
>> > This crash happens following this patch
>> > "1065da21e5df9d843d2c5165d5d576be000142a6" "ceph: stop copying to iter
>> > at EOF on sync reads".
>> >
>> > Per your fix Luis, it seems to address only the cases when i_size goes
>> > to zero but can happen anytime the `i_size` goes below  `off`.
>> > I propose fixing it this way:
>>
>> Hmm... you're probably right.  I didn't see this happening, but I guess it
>> could indeed happen.
>>
>> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> > index 4b8d59ebda00..19b084212fee 100644
>> > --- a/fs/ceph/file.c
>> > +++ b/fs/ceph/file.c
>> > @@ -1066,7 +1066,7 @@ ssize_t __ceph_sync_read(struct inode *inode,
>> > loff_t *ki_pos,
>> >         if (ceph_inode_is_shutdown(inode))
>> >                 return -EIO;
>> >
>> > -       if (!len)
>> > +       if (!len || !i_size)
>> >                 return 0;
>> >         /*
>> >          * flush any page cache pages in this range.  this
>> > @@ -1200,12 +1200,11 @@ ssize_t __ceph_sync_read(struct inode *inode,
>> > loff_t *ki_pos,
>> >                 }
>> >
>> >                 idx = 0;
>> > -               if (ret <= 0)
>> > -                       left = 0;
>>
>> Right now I don't have any means for testing this patch.  However, I don't
>> think this is completely correct.  By removing the above condition you're
>> discarding cases where an error has occurred (i.e. where ret is negative).
>
> I didn't discard it though :).
> I folded it into the `if` statement. I find the if else construct
> overly verbose and cumbersome.
>
> +                       left = (ret > 0) ? ret : 0;
>

Right, but with your patch, if 'ret < 0', we could still hit the first
branch instead of that one:

		if (off + ret > i_size)
			left = (i_size > off) ? i_size - off : 0;
		else
			left = (ret > 0) ? ret : 0;

Cheers,
-- 
Luís