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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADm8TenqC62ddsVdo3ipC_HL3S1w7jC-=54NbU3qWvJUvyB+Xg@mail.gmail.com>
Date: Fri, 19 Dec 2025 14:26:00 +0800
From: Tuo Li <islituo@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: "idryomov@...il.com" <idryomov@...il.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()

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.

Thanks again for your feedback!

Sincerely,
Tuo Li

>
> >               arg_map->args = kcalloc(arg_map->size, sizeof(*arg_map->args),
> >                                       GFP_NOIO);
> >               if (!arg_map->args) {
> >                       ret = -ENOMEM;
> >                       goto fail;
> >               }
> > +             arg_map->size = c->max_buckets;
> >
> >               ceph_decode_32_safe(p, end, num_buckets, e_inval);
> >               while (num_buckets--) {