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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0205e0b6-fad9-4519-adec-f1d1b30d9ef9@redhat.com>
Date: Fri, 23 Aug 2024 09:48:16 +0800
From: Xiubo Li <xiubli@...hat.com>
To: "Luis Henriques (SUSE)" <luis.henriques@...ux.dev>,
 Ilya Dryomov <idryomov@...il.com>
Cc: 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 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 ?

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;
>