[PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[Tuo Li]

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@gmail.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;
 		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--) {
-- 
2.43.0

Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[Viacheslav Dubeyko]

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

>  		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--) {

Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[Tuo Li]

Hi Slava,

On Fri, Dec 19, 2025 at 3:11 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.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@gmail.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--) {

Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[Ilya Dryomov]

On Fri, Dec 19, 2025 at 7:26 AM Tuo Li <islituo@gmail.com> wrote:
>
> Hi Slava,
>
> On Fri, Dec 19, 2025 at 3:11 AM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.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@gmail.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

Re: [PATCH] net: ceph: Fix a possible null-pointer dereference in decode_choose_args()

[Tuo Li]

On Fri, Dec 19, 2025 at 9:35 PM Ilya Dryomov <idryomov@gmail.com> wrote:
>
> On Fri, Dec 19, 2025 at 7:26 AM Tuo Li <islituo@gmail.com> wrote:
> >
> > Hi Slava,
> >
> > On Fri, Dec 19, 2025 at 3:11 AM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.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@gmail.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