[PATCH] cramfs: bound the XIP direct-mapping reads to the image size

[PATCH] cramfs: bound the XIP direct-mapping reads to the image size

[Bryam Vargas]

The physically-mapped (XIP) fast path dereferences the linear filesystem
image directly using offsets taken from the on-disk inode.  Unlike the
normal read path (cramfs_direct_read()), cramfs_get_block_range() and
cramfs_last_page_is_shared() do not check those offsets against the image
size before dereferencing:

	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) + pgoff * 4);
	first_block_addr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;

OFFSET(inode) (the inode data offset) and the block pointers are read
from the untrusted on-disk image.  A crafted cramfs image with the
CRAMFS_FLAG_EXT_BLOCK_POINTERS feature and an out-of-range inode offset
makes the block-pointer read -- and the subsequent tail-data scan in
cramfs_last_page_is_shared() -- dereference memory outside the mapped
image.  mmap()ing a regular file on such an image then faults in the
kernel (page fault in cramfs_physmem_mmap()).

Bound every linear-image dereference in the XIP path to sbi->size, the
same way cramfs_direct_read() already does, and fall back to the bounded
paging path on any out-of-range access.

Fixes: eddcd97659e3 ("cramfs: add mmap support")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced on v7.1-rc6 with KASAN, CONFIG_CRAMFS_MTD + MTD_MTDRAM (a small
RAM-backed mtd as the linear cramfs backing).  A crafted cramfs image with
CRAMFS_FLAG_EXT_BLOCK_POINTERS set and a regular file whose on-disk inode
offset is out of range, mounted over the mtd backend, faults the kernel when
the file is mmap()ed:

  BUG: unable to handle page fault for address ...
  #PF: supervisor read access in kernel mode
  RIP: ... cramfs_physmem_mmap+0x...
   __mmap_region
   mmap_region
   do_mmap
   vm_mmap_pgoff
   ksys_mmap_pgoff

A control image with an in-range offset maps and reads cleanly.  With this
patch the crafted image's XIP fast path detects the out-of-range offset and
falls back to the bounded paging path; no fault occurs.

 fs/cramfs/inode.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
index 4edbfccd0bbe..014072a65c5b 100644
--- a/fs/cramfs/inode.c
+++ b/fs/cramfs/inode.c
@@ -298,13 +298,22 @@ static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32 *pages)
 {
 	struct cramfs_sb_info *sbi = CRAMFS_SB(inode->i_sb);
 	int i;
-	u32 *blockptrs, first_block_addr;
+	u32 *blockptrs, first_block_addr, data_addr;
 
 	/*
 	 * We can dereference memory directly here as this code may be
 	 * reached only when there is a direct filesystem image mapping
 	 * available in memory.
+	 *
+	 * The block pointer array lives at OFFSET(inode) inside the image.
+	 * OFFSET() and the block pointers are taken from the (untrusted)
+	 * on-disk inode, so bound every access to the image the same way
+	 * cramfs_direct_read() does before dereferencing it.
 	 */
+	if (OFFSET(inode) > sbi->size ||
+	    ((u64)pgoff + *pages) * 4 > sbi->size - OFFSET(inode))
+		return 0;
+
 	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) + pgoff * 4);
 	first_block_addr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;
 	i = 0;
@@ -324,7 +333,14 @@ static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32 *pages)
 	} while (++i < *pages);
 
 	*pages = i;
-	return first_block_addr << CRAMFS_BLK_DIRECT_PTR_SHIFT;
+
+	/* The mapped data range must also lie within the image. */
+	data_addr = first_block_addr << CRAMFS_BLK_DIRECT_PTR_SHIFT;
+	if (data_addr > sbi->size ||
+	    (u64)*pages * PAGE_SIZE > sbi->size - data_addr)
+		return 0;
+
+	return data_addr;
 }
 
 #ifdef CONFIG_MMU
@@ -345,9 +361,21 @@ static bool cramfs_last_page_is_shared(struct inode *inode)
 	if (!partial)
 		return false;
 	last_page = inode->i_size >> PAGE_SHIFT;
+
+	/*
+	 * The block pointer and the tail data are read directly from the
+	 * image at offsets derived from the untrusted on-disk inode; bound
+	 * both accesses to the image.  On any overflow treat the last page
+	 * as shared so the caller falls back to the bounded paging path.
+	 */
+	if (OFFSET(inode) > sbi->size ||
+	    ((u64)last_page + 1) * 4 > sbi->size - OFFSET(inode))
+		return true;
 	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode));
 	blockaddr = blockptrs[last_page] & ~CRAMFS_BLK_FLAGS;
 	blockaddr <<= CRAMFS_BLK_DIRECT_PTR_SHIFT;
+	if (blockaddr > sbi->size || sbi->size - blockaddr < PAGE_SIZE)
+		return true;
 	tail_data = sbi->linear_virt_addr + blockaddr + partial;
 	return memchr_inv(tail_data, 0, PAGE_SIZE - partial) ? true : false;
 }
-- 
2.43.0

Re: [PATCH] cramfs: bound the XIP direct-mapping reads to the image size

[Christian Brauner]

> The physically-mapped (XIP) fast path dereferences the linear filesystem
> image directly using offsets taken from the on-disk inode.  Unlike the
> normal read path (cramfs_direct_read()), cramfs_get_block_range() and
> cramfs_last_page_is_shared() do not check those offsets against the image
> size before dereferencing:
> 
> 	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) + pgoff * 4);
> 	first_block_addr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;
> 
> OFFSET(inode) (the inode data offset) and the block pointers are read
> from the untrusted on-disk image.  A crafted cramfs image with the
> CRAMFS_FLAG_EXT_BLOCK_POINTERS feature and an out-of-range inode offset
> makes the block-pointer read -- and the subsequent tail-data scan in
> cramfs_last_page_is_shared() -- dereference memory outside the mapped
> image.  mmap()ing a regular file on such an image then faults in the
> kernel (page fault in cramfs_physmem_mmap()).
> 
> Bound every linear-image dereference in the XIP path to sbi->size, the
> same way cramfs_direct_read() already does, and fall back to the bounded
> paging path on any out-of-range access.
> 
> Fixes: eddcd97659e3 ("cramfs: add mmap support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 4edbfccd0bbe..014072a65c5b 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -298,13 +298,22 @@ static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32 *pages)
>  {
>  	struct cramfs_sb_info *sbi = CRAMFS_SB(inode->i_sb);
>  	int i;
> -	u32 *blockptrs, first_block_addr;
> +	u32 *blockptrs, first_block_addr, data_addr;
>  
>  	/*
>  	 * We can dereference memory directly here as this code may be
>  	 * reached only when there is a direct filesystem image mapping
>  	 * available in memory.
> +	 *
> +	 * The block pointer array lives at OFFSET(inode) inside the image.
> +	 * OFFSET() and the block pointers are taken from the (untrusted)
> +	 * on-disk inode, so bound every access to the image the same way
> +	 * cramfs_direct_read() does before dereferencing it.
>  	 */
> +	if (OFFSET(inode) > sbi->size ||
> +	    ((u64)pgoff + *pages) * 4 > sbi->size - OFFSET(inode))
> +		return 0;
> +
>  	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode) + pgoff * 4);
>  	first_block_addr = blockptrs[0] & ~CRAMFS_BLK_FLAGS;
>  	i = 0;
> @@ -324,7 +333,14 @@ static u32 cramfs_get_block_range(struct inode *inode, u32 pgoff, u32 *pages)
>  	} while (++i < *pages);
>  
>  	*pages = i;
> -	return first_block_addr << CRAMFS_BLK_DIRECT_PTR_SHIFT;
> +
> +	/* The mapped data range must also lie within the image. */
> +	data_addr = first_block_addr << CRAMFS_BLK_DIRECT_PTR_SHIFT;
> +	if (data_addr > sbi->size ||
> +	    (u64)*pages * PAGE_SIZE > sbi->size - data_addr)
> +		return 0;
> +
> +	return data_addr;
>  }
>  
>  #ifdef CONFIG_MMU
> @@ -345,9 +361,21 @@ static bool cramfs_last_page_is_shared(struct inode *inode)
>  	if (!partial)
>  		return false;
>  	last_page = inode->i_size >> PAGE_SHIFT;
> +
> +	/*
> +	 * The block pointer and the tail data are read directly from the
> +	 * image at offsets derived from the untrusted on-disk inode; bound
> +	 * both accesses to the image.  On any overflow treat the last page
> +	 * as shared so the caller falls back to the bounded paging path.
> +	 */
> +	if (OFFSET(inode) > sbi->size ||
> +	    ((u64)last_page + 1) * 4 > sbi->size - OFFSET(inode))
> +		return true;
>  	blockptrs = (u32 *)(sbi->linear_virt_addr + OFFSET(inode));
>  	blockaddr = blockptrs[last_page] & ~CRAMFS_BLK_FLAGS;
>  	blockaddr <<= CRAMFS_BLK_DIRECT_PTR_SHIFT;
> +	if (blockaddr > sbi->size || sbi->size - blockaddr < PAGE_SIZE)
> +		return true;
>  	tail_data = sbi->linear_virt_addr + blockaddr + partial;
>  	return memchr_inv(tail_data, 0, PAGE_SIZE - partial) ? true : false;


This screems a little for a shared litle static inline helper...

-- 
Christian Brauner <brauner@kernel.org>