linux-kernel - RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory free path

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]
Date:   Thu, 2 Aug 2018 15:54:50 +0000
From:   KY Srinivasan <kys@...rosoft.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        "Michael Kelley (EOSG)" <Michael.H.Kelley@...rosoft.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "olaf@...fle.de" <olaf@...fle.de>,
        "apw@...onical.com" <apw@...onical.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "marcelo.cerri@...onical.com" <marcelo.cerri@...onical.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>
Subject: RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic memory
 free path



> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@...cle.com>
> Sent: Wednesday, August 1, 2018 11:42 PM
> To: Michael Kelley (EOSG) <Michael.H.Kelley@...rosoft.com>
> Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> vkuznets@...hat.com; jasowang@...hat.com;
> marcelo.cerri@...onical.com; Stephen Hemminger
> <sthemmin@...rosoft.com>; KY Srinivasan <kys@...rosoft.com>
> Subject: Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Cleanup synic
> memory free path
> 
> On Wed, Aug 01, 2018 at 03:45:13PM -0700, mhkelley58@...il.com wrote:
> > From: Michael Kelley <mikelley@...rosoft.com>
> >
> > clk_evt memory is not being freed when the synic is shutdown
> > or when there is an allocation error.  Add the appropriate
> > kfree() call, along with a comment to clarify how the memory
> > gets freed after an allocation error.  Make the free path
> > consistent by removing checks for NULL since kfree() and
> > free_page() already do the check.
> >
> > Signed-off-by: Michael Kelley <mikelley@...rosoft.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> > ---
> >  drivers/hv/hv.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> > index 8d4fe0e..1fb9a6b 100644
> > --- a/drivers/hv/hv.c
> > +++ b/drivers/hv/hv.c
> > @@ -240,6 +240,10 @@ int hv_synic_alloc(void)
> >
> >  	return 0;
> >  err:
> > +	/*
> > +	 * Any memory allocations that succeeded will be freed when
> > +	 * the caller cleans up by calling hv_synic_free()
> > +	 */
> >  	return -ENOMEM;
> >  }
> >
> > @@ -252,12 +256,10 @@ void hv_synic_free(void)
> >         for_each_present_cpu(cpu) {
> >  		struct hv_per_cpu_context *hv_cpu
> >  			= per_cpu_ptr(hv_context.cpu_context, cpu);
> >
> > -		if (hv_cpu->synic_event_page)
> > -			free_page((unsigned long)hv_cpu-
> >synic_event_page);
> > -		if (hv_cpu->synic_message_page)
> > -			free_page((unsigned long)hv_cpu-
> >synic_message_page);
> > -		if (hv_cpu->post_msg_page)
> > -			free_page((unsigned long)hv_cpu-
> >post_msg_page);
> > +		kfree(hv_cpu->clk_evt);
> > +		free_page((unsigned long)hv_cpu->synic_event_page);
> > +		free_page((unsigned long)hv_cpu->synic_message_page);
> > +		free_page((unsigned long)hv_cpu->post_msg_page);
> 
> This looks buggy.
> 
> We can pass NULLs to free_page() so that's fine.  So the error handling
> assumes
> that hv_cpu->clk_evt is either NULL or allocated.  Here is how it is allocated:
> 
>    189  int hv_synic_alloc(void)
>    190  {
>    191          int cpu;
>    192
>    193          hv_context.hv_numa_map = kcalloc(nr_node_ids, sizeof(struct
> cpumask),
>    194                                           GFP_KERNEL);
>    195          if (hv_context.hv_numa_map == NULL) {
>    196                  pr_err("Unable to allocate NUMA map\n");
>    197                  goto err;
>    198          }
>    199
>    200          for_each_present_cpu(cpu) {
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We loop over each CPU.
> 
>    201                  struct hv_per_cpu_context *hv_cpu
>    202                          = per_cpu_ptr(hv_context.cpu_context, cpu);
>    203
>    204                  memset(hv_cpu, 0, sizeof(*hv_cpu));
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> We set this cpu memory to NULL.
> 
>    205                  tasklet_init(&hv_cpu->msg_dpc,
>    206                               vmbus_on_msg_dpc, (unsigned long) hv_cpu);
>    207
>    208                  hv_cpu->clk_evt = kzalloc(sizeof(struct clock_event_device),
>    209                                            GFP_KERNEL);
>    210                  if (hv_cpu->clk_evt == NULL) {
>                             ^^^^^^^^^^^^^^^^^^^^^^^
> Let's assume this fails on the first iteration through the loop.  We
> haven't memset the next cpu to NULL or allocated it.  But we loop over
> all the cpus in the error handling.  Since we didn't set everything to NULL in
> hv_synic_free() then it seems like this could be a double free.  It's possible I
> am misreading the code, but either it's buggy or the memset() can be
> removed.
> 
> This is a very typical bug for this style of error handling where we free
> things which were never allocated.

Thanks Dan! We will fix this issue soon.

K. Y
> 
>    211                          pr_err("Unable to allocate clock event device\n");
>    212                          goto err;
>    213                  }
> 
> regards,
> dan carpenter