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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CADm8TemRooH7BBpzW6E+S5yjMK=JG0gNARXJmttRX0Bp7ByQ9g@mail.gmail.com>
Date: Sat, 20 Dec 2025 14:24:56 +0800
From: Tuo Li <islituo@...il.com>
To: Ilya Dryomov <idryomov@...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 9:35 PM Ilya Dryomov <idryomov@...il.com> wrote:
>
> 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

Hi Ilya,

Thanks a lot for the detailed suggestion.

I think that making free_choose_arg_map() more resilient is better.

I will rework the fix accordingly and send a v2 patch shortly.

Thanks again for the review and guidance.

Sincerely,
Tuo