Re: [bug report] ceph: decode interval_sets for delegated inos

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2024-12-03 at 11:25 +0300, Dan Carpenter wrote:
> On Tue, Dec 03, 2024 at 11:19:25AM +0300, Dan Carpenter wrote:
> > Hello Jeff Layton,
> > 
> > Commit d48464878708 ("ceph: decode interval_sets for delegated inos")
> > from Nov 15, 2019 (linux-next), leads to the following Smatch static
> > checker warning:
> > 
> > 	fs/ceph/mds_client.c:644 ceph_parse_deleg_inos()
> > 	warn: potential user controlled sizeof overflow 'sets * 2 * 8' '0-u32max * 8'
> > 
> > fs/ceph/mds_client.c
> >     637 static int ceph_parse_deleg_inos(void **p, void *end,
> >     638                                  struct ceph_mds_session *s)
> >     639 {
> >     640         u32 sets;
> >     641 
> >     642         ceph_decode_32_safe(p, end, sets, bad);
> >                                             ^^^^
> > set to user data here.
> > 
> >     643         if (sets)
> > --> 644                 ceph_decode_skip_n(p, end, sets * 2 * sizeof(__le64), bad);
> >                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^
> > This is safe on 64bit but on 32bit systems it can integer overflow/wrap.
> > 

In practice, the MDS is only sending a few sets at a time, so this is
an unlikely overflow. Probably, the right fix is to just clamp "sets"
at some value. Maybe 1K or 1M or so? I'd probably rather the Ceph folks
propose a patch for this since I'm not heavily involved there these
days.

> Smatch has similar warnings in ceph_mdsmap_decode().
> 
> fs/ceph/mdsmap.c:228 ceph_mdsmap_decode() warn: potential user controlled sizeof overflow 'num_export_targets * 4' '0-u32max * 4'
> fs/ceph/mdsmap.c:280 ceph_mdsmap_decode() warn: potential user controlled sizeof overflow '8 * (n + 1)' '8 * s32min-s32max'
> fs/ceph/mdsmap.c:337 ceph_mdsmap_decode() warn: potential user controlled sizeof overflow '4 * n' '4 * 0-u32max'
> fs/ceph/mdsmap.c:339 ceph_mdsmap_decode() warn: potential user controlled sizeof overflow '4 * n' '4 * 0-u32max'
> 

Yeah, the mdsmap decoding definitely has some warts. It'd be good to
fix up some of those too so that they can never be a problem.
-- 
Jeff Layton <jlayton@kernel.org>