[bug report] ceph: decode interval_sets for delegated inos

[bug report] ceph: decode interval_sets for delegated inos

[Dan Carpenter]

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.

    645         return 0;
    646 bad:
    647         return -EIO;
    648 }
    649 
    650 u64 ceph_get_deleg_ino(struct ceph_mds_session *s)
    651 {
    652         return 0;
    653 }
    654 

regards,
dan carpenter

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

[Dan Carpenter]

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

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'

regards,
dan carpenter

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

[Jeff Layton]

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>

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

[Alex Elder]

On 12/3/24 2:19 AM, 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.

So the point of this is that "sets" is u32, and because that is
multiplied by 16 when passed to ceph_decode_skip_n(), the result
could exceed 32 bits?  I.e., would this address it?

	if (sets) {
	    size_t scale = 2 * sizeof(__le64);

	    if (sets < SIZE_MAX / scale)
		ceph_decode_skip_n(p, end, sets * scale, bad);
	    else
		goto bad;
	}

					-Alex


>      645         return 0;
>      646 bad:
>      647         return -EIO;
>      648 }
>      649
>      650 u64 ceph_get_deleg_ino(struct ceph_mds_session *s)
>      651 {
>      652         return 0;
>      653 }
>      654
> 
> regards,
> dan carpenter
> 

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

[Dan Carpenter]

On Tue, Dec 03, 2024 at 11:06:50AM -0600, Alex Elder wrote:
> On 12/3/24 2:19 AM, 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.
> 
> So the point of this is that "sets" is u32, and because that is
> multiplied by 16 when passed to ceph_decode_skip_n(), the result
> could exceed 32 bits?  I.e., would this address it?
> 
> 	if (sets) {
> 	    size_t scale = 2 * sizeof(__le64);
> 
> 	    if (sets < SIZE_MAX / scale)
> 		ceph_decode_skip_n(p, end, sets * scale, bad);
> 	    else
> 		goto bad;
> 	}
> 

Yes, that works.  I don't know if there are any static checker warnings which
will complain that the "sets < SIZE_MAX / scale" is always true on 64 bit.  I
don't think there is?

regards,
dan carpenter

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

[Alex Markuze]

I assume a Coccinelle patch can be written, if one doesn't exist yet.

On Tue, Dec 3, 2024 at 8:29 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Tue, Dec 03, 2024 at 11:06:50AM -0600, Alex Elder wrote:
> > On 12/3/24 2:19 AM, 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.
> >
> > So the point of this is that "sets" is u32, and because that is
> > multiplied by 16 when passed to ceph_decode_skip_n(), the result
> > could exceed 32 bits?  I.e., would this address it?
> >
> >       if (sets) {
> >           size_t scale = 2 * sizeof(__le64);
> >
> >           if (sets < SIZE_MAX / scale)
> >               ceph_decode_skip_n(p, end, sets * scale, bad);
> >           else
> >               goto bad;
> >       }
> >
>
> Yes, that works.  I don't know if there are any static checker warnings which
> will complain that the "sets < SIZE_MAX / scale" is always true on 64 bit.  I
> don't think there is?
>
> regards,
> dan carpenter
>
>

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

[Alex Markuze]

Dan, how are you running smatch?
I've been looking at smatch warnings/errors and don't get this error.
Do you have a custom smatch checker?

On Wed, Dec 4, 2024 at 2:50 PM Alex Markuze <amarkuze@redhat.com> wrote:
>
> I assume a Coccinelle patch can be written, if one doesn't exist yet.
>
> On Tue, Dec 3, 2024 at 8:29 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Tue, Dec 03, 2024 at 11:06:50AM -0600, Alex Elder wrote:
> > > On 12/3/24 2:19 AM, 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.
> > >
> > > So the point of this is that "sets" is u32, and because that is
> > > multiplied by 16 when passed to ceph_decode_skip_n(), the result
> > > could exceed 32 bits?  I.e., would this address it?
> > >
> > >       if (sets) {
> > >           size_t scale = 2 * sizeof(__le64);
> > >
> > >           if (sets < SIZE_MAX / scale)
> > >               ceph_decode_skip_n(p, end, sets * scale, bad);
> > >           else
> > >               goto bad;
> > >       }
> > >
> >
> > Yes, that works.  I don't know if there are any static checker warnings which
> > will complain that the "sets < SIZE_MAX / scale" is always true on 64 bit.  I
> > don't think there is?
> >
> > regards,
> > dan carpenter
> >
> >

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

[Dan Carpenter]

On Wed, Dec 04, 2024 at 03:12:31PM +0200, Alex Markuze wrote:
> Dan, how are you running smatch?
> I've been looking at smatch warnings/errors and don't get this error.
> Do you have a custom smatch checker?

This is released code, but there are two possible explanations for why you
wouldn't see the warning:

1)  Are you using the cross function database?  Each time you rebuild the
    database then it adds another layer to the call tree.  EDIT: But
    actually in this case, it doesn't matter because Smatch hardcodes
    ceph_decode_64() as returning user data so the database isn't
    required.

2) This bug only affects 32bit .configs and I suspect you're building for
   64bit kernels.

regards,
dan carpenter