linux-kernel - Re: [PATCH v9 3/4] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver

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: <20160715094541.GE4655@leverpostej>
Date:	Fri, 15 Jul 2016 10:45:42 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Tai Tri Nguyen <ttnguyen@....com>
Cc:	Joe Perches <joe@...ches.com>, Will Deacon <will.deacon@....com>,
	catalin.marinas@....com,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Duc Dang <dhdang@....com>, LKML <linux-kernel@...r.kernel.org>,
	devicetree@...r.kernel.org,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	patches <patches@....com>
Subject: Re: [PATCH v9 3/4] perf: xgene: Add APM X-Gene SoC Performance
 Monitoring Unit driver

On Thu, Jul 14, 2016 at 11:05:48AM -0700, Tai Tri Nguyen wrote:
> Hi Joe,
> 
> On Thu, Jul 14, 2016 at 10:54 AM, Tai Tri Nguyen <ttnguyen@....com> wrote:
> > Hi Joe,
> >
> > On Thu, Jul 14, 2016 at 10:47 AM, Joe Perches <joe@...ches.com> wrote:
> >> On Thu, 2016-07-14 at 10:27 -0700, Tai Nguyen wrote:
> >>> This patch adds a driver for the SoC-wide (AKA uncore) PMU hardware
> >>> found in APM X-Gene SoCs.
> >>
> >> trivia:
> >>
> >>> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> >> []
> >>> +struct xgene_pmu_dev_ctx {
> >>> +     char *name;
> >>> +     struct list_head next;
> >>> +     struct xgene_pmu_dev *pmu_dev;
> >>> +     struct hw_pmu_info inf;
> >>> +};
> >>
> >> Probably better to use something like
> >>         char    name[20];
> >> as the kasprintf can fail and this doesn't
> >> seem to be freed anywhere.
> >
> > Okay. I'll fix it shortly.
> >
> 
> I take it back.
> I refer many other drivers using kasprintf and they do the same way I do.
> Can you please check it again?

Joe is correct that you allocate a string with kasprintf, and this never
gets freed, even if the driver is removed. Thus, memory may be leaked.

If other drivers do the same, they are similarly wrong.

Even if this is a rare case, it's not good practice to leave allocations
unbalanced. So please fix this.

If you don't want to change the struct, another option is to use
devm_kasprintf. However, I suspect with all the accounting data
structures that will take up more space.

Thanks,
Mark.