Re: [PATCH v2] ceph: fix OOB read in decode_watchers() via missing bounds check

[Viacheslav Dubeyko]

On Fri, 2026-05-22 at 06:19 -0400, Pavitra Jha wrote:
> ceph_start_decoding() validates that struct_len bytes remain in the
> buffer after the encoding header, but accepts struct_len=0 as valid:
> ceph_decode_need(p, end, 0, bad) always passes. When a malicious or
> compromised OSD sends an obj_list_watch_response_t reply with
> struct_len=0, ceph_start_decoding() returns success with p == end,
> leaving zero bytes guaranteed for subsequent reads.
> 
> The immediately following ceph_decode_32(p) in decode_watchers() has
> no preceding bounds check. With p == end this is a 4-byte read past
> the validated buffer boundary. The garbage value is then passed
> directly to kzalloc_objs() as the watcher count.
> 
> The sibling function decode_watcher() already uses the safe variants
> (ceph_decode_copy_safe, ceph_decode_64_safe, ceph_decode_skip_32)
> after its own ceph_start_decoding() call. decode_watchers() is the
> only site that uses the bare variant, confirming an oversight.
> 
> Fix by replacing ceph_decode_32(p) with ceph_decode_32_safe(p, end,
> *num_watchers, e_inval), consistent with the established pattern.
> 
> KASAN report (kernel 7.0.0-rc7, QEMU/x86_64, KASLR disabled):
> 
>   [   72.047085] ceph_oob_poc: buf=ffff8880085936c8 end=ffff8880085936ce
>   [   72.048685] ceph_oob_poc: ceph_start_decoding OK: struct_v=1
>   struct_len=0 p==end: 1
>   [   72.049477] ceph_oob_poc: triggering OOB read past slab boundary...
>   [   72.050699] ==================================================
>   [   72.051427] BUG: KASAN: slab-out-of-bounds in
>   ceph_oob_init+0x128/0xff0 [ceph_oob_poc]
>   [   72.051427] Read of size 4 at addr ffff8880085936ce by task insmod/61
>   [   72.051427] CPU: 0 UID: 0 PID: 61 Comm: insmod Tainted: G O
>   [   72.051427]  7.0.0-rc7-g9c2abf69da83-dirty #14 PREEMPT(lazy)
>   [   72.051427] Call Trace:
>   [   72.051427]  dump_stack_lvl+0x4d/0x70
>   [   72.051427]  print_report+0x170/0x4f3
>   [   72.051427]  kasan_report+0xda/0x110
>   [   72.051427]  kasan_check_range+0x125/0x200
>   [   72.051427]  ceph_oob_init+0x128/0xff0 [ceph_oob_poc]
>   [   72.051427]  do_one_initcall+0x9a/0x310
>   [   72.051427]  do_init_module+0x186/0x410
>   [   72.051427]  load_module+0x2ba7/0x2e50
>   [   72.051427]  init_module_from_file+0x15c/0x180
>   [   72.051427]  idempotent_init_module+0x19f/0x430
>   [   72.051427]  __x64_sys_finit_module+0x78/0xc0
>   [   72.051427]  do_syscall_64+0xe2/0x570
>   [   72.051427]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>   [   72.051427] The buggy address belongs to the object at ffff8880085936c8
>   [   72.051427]  which belongs to the cache kmalloc-8 of size 8
>   [   72.051427] The buggy address is located 0 bytes to the right of
>   [   72.051427]  allocated 6-byte region [ffff8880085936c8, ffff8880085936ce)
>   [   72.051427] Memory state around the buggy address:
>   [   72.051427] >ffff888008593680: fc fc fc fc fc fc fc fc fc 06 fc fc fc fc fc fc
>   [   72.051427]                                               ^
>   [   72.051427] ==================================================
>   [   72.129720] ceph_oob_poc: num_watchers=3435973836 (OOB garbage)
> 
>   0xCCCCCCCC (3435973836) is KASAN redzone poison, confirming the read
>   landed in the slab redzone immediately past the 6-byte allocation.
> 
> Attacker model: a malicious or compromised OSD in a multi-tenant Ceph
> deployment (e.g. cloud) can trigger this against any kernel client
> that calls CEPH_OSD_OP_LIST_WATCHERS, without any further privileges
> beyond OSD session establishment.
> 
> Fixes: a4ed38d7a180 ("libceph: support for CEPH_OSD_OP_LIST_WATCHERS")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Pavitra Jha <jhapavitra98@xxxxxxxxx>
> ---
> v2: Correct commit message. v1 overstated the impact:
> 
>   - "unbounded alloc": retracted. kzalloc_objs() uses size_mul()
>     internally which returns SIZE_MAX on overflow, causing kmalloc
>     to return NULL. The large garbage value from the OOB read will
>     simply fail allocation with -ENOMEM.
> 
>   - "decode_watcher() writing attacker-controlled data into it":
>     retracted. ceph_start_decoding() calls ceph_decode_need() for
>     its 6-byte header, which catches p==end and returns -ERANGE
>     before any copy occurs. Verified with a follow-up KASAN harness.
> 
>   The fix itself is unchanged.
> ---
>  net/ceph/osd_client.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 2ff00070c..0148e4c40 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -5030,7 +5030,7 @@ static int decode_watchers(void **p, void *end,
>  	if (ret)
>  		return ret;
>  
> -	*num_watchers = ceph_decode_32(p);
> +	ceph_decode_32_safe(p, end, *num_watchers, e_inval);
>  	*watchers = kzalloc_objs(**watchers, *num_watchers, GFP_NOIO);
>  	if (!*watchers)
>  		return -ENOMEM;
> @@ -5044,6 +5044,9 @@ static int decode_watchers(void **p, void *end,
>  	}
>  
>  	return 0;
> +
> +e_inval:
> +	return -EINVAL;

Maybe, I not completely like the e_inval name. :) Usually, it is used bad name.
But it's completely not critical at all.

Also, I am not completely sure that -EINVAL is correct error code. Usually,
EINVAL is used for invalid argument. Here, we analyze the reply's content and it
sounds like not input argument. And, finally, we cannot allocate memory. So,
maybe, -ENOMEM is more proper error code. However, it is not critical remark to.

>  }
>  
>  /*

The fix makes sense and it looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>

Thanks,
Slava.