[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87ikvrhfa7.fsf@linux.dev>
Date: Fri, 23 Aug 2024 08:25:20 +0100
From: Luis Henriques <luis.henriques@...ux.dev>
To: Xiubo Li <xiubli@...hat.com>
Cc: Ilya Dryomov <idryomov@...il.com>, ceph-devel@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] ceph: fix out-of-bound array access when doing a
file read
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@...ux.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
Cheers,
--
Luís
>
> Thanks
>
> - Xiubo
>
>> left = i_size - off;
>> else
>> left = ret;
>> @@ -1210,6 +1210,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
>> size_t plen, copied;
>> plen = min_t(size_t, left, PAGE_SIZE - page_off);
>> + WARN_ON_ONCE(idx >= num_pages);
>> SetPageUptodate(pages[idx]);
>> copied = copy_page_to_iter(pages[idx++],
>> page_off, plen, to);
>> @@ -1234,7 +1235,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos,
>> }
>> if (ret > 0) {
>> - if (off >= i_size) {
>> + if ((i_size >= *ki_pos) && (off >= i_size)) {
>> *retry_op = CHECK_EOF;
>> ret = i_size - *ki_pos;
>> *ki_pos = i_size;
>>
>
Powered by blists - more mailing lists