NACK: [SRU][Focal][PATCH 1/1] relayfs: fix out-of-bounds access in relay_file_read

[Yuxuan Luo]

v2 incoming.


On 7/11/23 10:16, Yuxuan Luo wrote:
> From: Zhang Zhengming <zhang.zhengming at h3c.com>
>
> There is a crash in relay_file_read, as the var from
> point to the end of last subbuf.
>
> The oops looks something like:
> pc : __arch_copy_to_user+0x180/0x310
> lr : relay_file_read+0x20c/0x2c8
> Call trace:
>   __arch_copy_to_user+0x180/0x310
>   full_proxy_read+0x68/0x98
>   vfs_read+0xb0/0x1d0
>   ksys_read+0x6c/0xf0
>   __arm64_sys_read+0x20/0x28
>   el0_svc_common.constprop.3+0x84/0x108
>   do_el0_svc+0x74/0x90
>   el0_svc+0x1c/0x28
>   el0_sync_handler+0x88/0xb0
>   el0_sync+0x148/0x180
>
> We get the condition by analyzing the vmcore:
>
> 1). The last produced byte and last consumed byte
>      both at the end of the last subbuf
>
> 2). A softirq calls function(e.g __blk_add_trace)
>      to write relay buffer occurs when an program is calling
>      relay_file_read_avail().
>
>          relay_file_read
>                  relay_file_read_avail
>                          relay_file_read_consume(buf, 0, 0);
>                          //interrupted by softirq who will write subbuf
>                          ....
>                          return 1;
>                  //read_start point to the end of the last subbuf
>                  read_start = relay_file_read_start_pos
>                  //avail is equal to subsize
>                  avail = relay_file_read_subbuf_avail
>                  //from  points to an invalid memory address
>                  from = buf->start + read_start
>                  //system is crashed
>                  copy_to_user(buffer, from, avail)
>
> Link: https://lkml.kernel.org/r/20230419040203.37676-1-zhang.zhengming@h3c.com
> Fixes: 8d62fdebdaf9 ("relay file read: start-pos fix")
> Signed-off-by: Zhang Zhengming <zhang.zhengming at h3c.com>
> Reviewed-by: Zhao Lei <zhao_lei1 at hoperun.com>
> Reviewed-by: Zhou Kete <zhou.kete at h3c.com>
> Reviewed-by: Pengcheng Yang <yangpc at wangsu.com>
> Cc: Jens Axboe <axboe at kernel.dk>
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> (backported from commit 43ec16f1450f4936025a9bdf1a273affdb9732c1)
> [yuxuan.luo: modify the read_pos = consumed * subbuf_size +
>   buf->bytes_consumed; line directly.
> ]
> CVE-2023-3268
> Signed-off-by: Yuxuan Luo <yuxuan.luo at canonical.com>
> ---
>   kernel/relay.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/relay.c b/kernel/relay.c
> index 9b1cfcd8dc6b1..29ac38e42a297 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -1081,7 +1081,8 @@ static size_t relay_file_read_start_pos(size_t read_pos,
>   	size_t consumed = buf->subbufs_consumed % n_subbufs;
>   
>   	if (!read_pos)
> -		read_pos = consumed * subbuf_size + buf->bytes_consumed;
> +		read_pos = (consumed * subbuf_size + buf->bytes_consumed)
> +			 % (n_subbufs * subbuf_size);
>   	read_subbuf = read_pos / subbuf_size;
>   	padding = buf->padding[read_subbuf];
>   	padding_start = (read_subbuf + 1) * subbuf_size - padding;