linux-kernel - Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOi1vP9Ho5eObbH4o-_66M9RFks=g7OUNnUyPUFMq1CgnBxfKQ@mail.gmail.com>
Date: Fri, 19 Dec 2025 14:35:11 +0100
From: Ilya Dryomov <idryomov@...il.com>
To: Tuo Li <islituo@...il.com>
Cc: Viacheslav Dubeyko <Slava.Dubeyko@....com>, Xiubo Li <xiubli@...hat.com>, 
	"ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

On Fri, Dec 19, 2025 at 7:26 AM Tuo Li <islituo@...il.com> wrote:
>
> Hi Slava,
>
> On Fri, Dec 19, 2025 at 3:11 AM Viacheslav Dubeyko
> <Slava.Dubeyko@....com> wrote:
> >
> > On Thu, 2025-12-18 at 15:56 +0800, Tuo Li wrote:
> > > In decode_choose_args(), arg_map->size is updated before memory is
> > > allocated for arg_map->args using kcalloc(). If kcalloc() fails, execution
> > > jumps to the fail label, where free_choose_arg_map() is called to release
> > > resources. However, free_choose_arg_map() unconditionally iterates over
> > > arg_map->args using arg_map->size, which can lead to a NULL pointer
> > > dereference when arg_map->args is NULL:
> > >
> > >   for (i = 0; i < arg_map->size; i++) {
> > >     struct crush_choose_arg *arg = &arg_map->args[i];
> > >
> > >       for (j = 0; j < arg->weight_set_size; j++)
> > >         kfree(arg->weight_set[j].weights);
> > >     kfree(arg->weight_set);
> > >       kfree(arg->ids);
> > >   }
> > >
> > > To prevent this potential NULL pointer dereference, move the assignment to
> > > arg_map->size to after successful allocation of arg_map->args. This ensures
> > > that when allocation fails, arg_map->size remains zero and the loop in
> > > free_choose_arg_map() is not executed.
> > >
> > > Signed-off-by: Tuo Li <islituo@...il.com>
> > > ---
> > >  net/ceph/osdmap.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> > > index d245fa508e1c..f67a87b3a7c8 100644
> > > --- a/net/ceph/osdmap.c
> > > +++ b/net/ceph/osdmap.c
> > > @@ -363,13 +363,13 @@ static int decode_choose_args(void **p, void *end, struct crush_map *c)
> > >
> > >               ceph_decode_64_safe(p, end, arg_map->choose_args_index,
> > >                                   e_inval);
> > > -             arg_map->size = c->max_buckets;
> >
> > The arg_map->size defines the size of memory allocation. If you remove the
> > assignment here, then which size kcalloc() will allocate. I assume we could have
> > two possible scenarios here: (1) arg_map->size is equal to zero -> no allocation
> > happens, (2) arg_map->size contains garbage value -> any failure could happen.
> >
> > Have you reproduced the declared issue that you are trying to fix? Are you sure
> > that your patch can fix the issue? Have you tested your patch at all?
> >
> > Thanks,
> > Slava.
>
> Thanks for your careful review.
>
> I found this issue through static analysis. It is indeed hard to reproduce
> in practice, since intentionally triggering a kcalloc() failure is not
> easy, but I think the NULL-dereference on the error path is theoretically
> possible.
>
> You are absolutely right that my original fix is incorrect, as kcalloc()
> relies on arg_map->size, and moving the assignment can introduce a new
> bug. I am so sorry for the oversight.
>
> After reading the code again, I see two possible approaches:
>
> 1. Keep the assignment to arg_map->size after the allocation, but use
> c->max_buckets directly as the allocation size when calling kcalloc().
>
> 2. Keep the assignment before kcalloc(), but explicitly set
> arg_map->size = 0 before jumping to fail, so that free_choose_arg_map()
> does not iterate over a NULL pointer.
>
> I would appreciate your thoughts on which approach is preferable, or if
> there is a better alternative.

Hi Tuo,

I think it would be better to make free_choose_arg_map() more resilient
instead.  The pattern that is followed elsewhere in the surrounding code
(e.g. crush_destroy()) is that the size of the array is considered valid
only the array itself is allocated.  Something like this:

static void free_choose_arg_map(struct crush_choose_arg_map *arg_map)
{
        int i, j;

        if (!arg_map)
                return;

        WARN_ON(!RB_EMPTY_NODE(&arg_map->node));

        if (arg_map->args) {
                for (i = 0; i < arg_map->size; i++) {
                        struct crush_choose_arg *arg = &arg_map->args[i];
                        if (arg->weight_set) {
                                for (j = 0; j < arg->weight_set_size; j++)
                                        kfree(arg->weight_set[j].weights);
                                kfree(arg->weight_set);
                        }
                        kfree(arg->ids);
                }
                kfree(arg_map->args);
        }
        kfree(arg_map);
}

Thanks,

                Ilya