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

[Xiubo Li <xiubli@redhat.com>]

On 8/23/24 15:25, Luis Henriques wrote:
> On Fri, Aug 23 2024, Xiubo Li wrote:
>
>> On 8/22/24 23:01, Luis Henriques (SUSE) wrote:
>>> If, while doing a read, the inode is updated and the size is set to zero,
>>> __ceph_sync_read() may not be able to handle it.  It is thus easy to hit a
>>> NULL pointer dereferrence by continuously reading a file while, on another
>>> client, we keep truncating and writing new data into it.
>>>
>>> This patch fixes the issue by adding extra checks to avoid integer overflows
>>> for the case of a zero size inode.  This will prevent the loop doing page
>>> copies from running and thus accessing the pages[] array beyond num_pages.
>>>
>>> Link: https://tracker.ceph.com/issues/67524
>>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>>> ---
>>> Hi!
>>>
>>> Please note that this patch is only lightly tested and, to be honest, I'm
>>> not sure if this is the correct way to fix this bug.  For example, if the
>>> inode size is 0, then maybe ceph_osdc_wait_request() should have returned
>>> 0 and the problem would be solved.  However, it seems to be returning the
>>> size of the reply message and that's not something easy to change.  Or maybe
>>> I'm just reading it wrong.  Anyway, this is just an RFC to see if there's
>>> other ideas.
>>>
>>> Also, the tracker contains a simple testcase for crashing the client.
>>>
>>>    fs/ceph/file.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>> index 4b8d59ebda00..dc23d5e5b11e 100644
>>> --- a/fs/ceph/file.c
>>> +++ b/fs/ceph/file.c
>>> @@ -1200,9 +1200,9 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
>>>    		}
>>>      		idx = 0;
>>> -		if (ret <= 0)
>>> +		if ((ret <= 0) || (i_size == 0))
>> Hi Luis,
>>
>> This change looks incorrect to me.
>>
>> As I mentioned before when the 'IFILE' lock is in MIX state the 'Frw' caps could
>> be issued to multiple clients at the same time. Which means the file could be
>> updated by another client and the local 'i_size' may haven't been changed in
>> time. So in this case the 'ret' will be larger than '0' and the 'i_size' could
>> be '0'.
>>
>>
>>>    			left = 0;
>>> -		else if (off + ret > i_size)
>>> +		else if ((i_size >= off) && (off + ret > i_size))
>> And the 'off' also could equal to little than the 'i_size'.
>>
>> BTW, could you reproduce the crash issue ?
> Yes, 100% reproducible :-)
>
> See https://tracker.ceph.com/issues/67524

Okay, Let me have a try about this.

Thanks


>
> Cheers,