Re: intel_cacheinfo: potential NULL dereference?

[Borislav Petkov <bp@amd64.org>]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Tue, Jun 22, 2010 at 01:09:40PM -0400

> On 06/22/2010 07:11 AM, Jiri Slaby wrote:
> > On 06/22/2010 03:08 PM, Borislav Petkov wrote:
> >> From: Jiri Slaby <jirislaby@gmail.com>
> >> Date: Tue, Jun 22, 2010 at 07:20:14AM -0400
> >>
> >>> On 06/22/2010 01:18 PM, Jiri Slaby wrote:
> >>>> Stanse found, that this_leaf->l3 is dereferenced at <<1>>, but checked
> >>>> for being NULL at <<2>>. Is the check superfluous or the dev assignment
> >>>> should go after the check?
> >>>
> >>> Oh, and I have another report with same symptoms for show_cache_disable.
> >>
> >> Right, so I have a patch in tip/x86/cpu
> >> (8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0) which reorganizes
> >> and cleans up that code. With it, all possible checks land in
> >> amd_check_l3_disable() and if they have all been passed, the PCI dev is
> >> guaranteed to be properly set. So no need for sprinkling additional NULL
> >> checks in the code.
> >>
> >> How's that?
> > 
> > Looks good.
> > 
> 
> Do we need a patch for the existing code to go into -linus and -stable,
> though?

Right, so this is not such a bad idea, actually. Here's a small fix,
just in case. This should go to -linus so that the issue is patched up
for .35 time. No need for -stable since this code came into the last
merge window.

The 8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0 in tip/x86/cpu was meant
for the .36 merge window so I'll readjust it relative to the small fix
below.

---
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Tue, 22 Jun 2010 21:06:51 +0200
Subject: [PATCH] x86, cacheinfo: Move dereferencing after NULL check

The sysfs handlers {show,store}_cache_disable() should dereference the
pci dev pointer after having checked the previous l3 pointer in the
chain first.

Spotted-by: Jiri Slaby <jirislaby@gmail.com>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 33eae20..a7e358f 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -399,16 +399,15 @@ amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
 static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
 				  unsigned int slot)
 {
-	struct pci_dev *dev = this_leaf->l3->dev;
 	unsigned int reg = 0;
 
 	if (!this_leaf->l3 || !this_leaf->l3->can_disable)
 		return -EINVAL;
 
-	if (!dev)
+	if (!this_leaf->l3->dev)
 		return -EINVAL;
 
-	pci_read_config_dword(dev, 0x1BC + slot * 4, &reg);
+	pci_read_config_dword(this_leaf->l3->dev, 0x1BC + slot * 4, &reg);
 	return sprintf(buf, "0x%08x\n", reg);
 }
 
@@ -456,7 +455,6 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 				   const char *buf, size_t count,
 				   unsigned int slot)
 {
-	struct pci_dev *dev = this_leaf->l3->dev;
 	int cpu = cpumask_first(to_cpumask(this_leaf->shared_cpu_map));
 	unsigned long val = 0;
 
@@ -469,7 +467,7 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!dev)
+	if (!this_leaf->l3->dev)
 		return -EINVAL;
 
 	if (strict_strtoul(buf, 10, &val) < 0)
-- 
1.6.4.4


-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632