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
| ||
|
Date: Thu, 6 May 2010 15:46:21 +0200 (CEST) From: Jiri Kosina <jkosina@...e.cz> To: Borislav Petkov <borislav.petkov@....com> Cc: Greg KH <gregkh@...e.de>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "stable@...nel.org" <stable@...nel.org>, "stable-review@...nel.org" <stable-review@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, Andrew Morton <akpm@...ux-foundation.org>, Alan Cox <alan@...rguk.ukuu.org.uk>, "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...e.hu>, Thomas Renninger <trenn@...e.de>, Jiri Benc <jbenc@...e.cz>, "Herrmann3, Andreas" <Andreas.Herrmann3@....com>, "Ostrovsky, Boris" <Boris.Ostrovsky@....com> Subject: Re: [113/197] x86, cacheinfo: Calculate L3 indices On Thu, 6 May 2010, Borislav Petkov wrote: > > The point is though, that we really should be checking for return value of > > node_to_k8_nb_misc() as it can legitimately return NULL, can't it? (and > > other places, such as show_cache_disable() and store_cache_disable(), are > > checking for this being NULL properly). > > Well, I don't like sprinkling NULL ptr checks all over the place - this > makes the code unreadable and means that there's something wrong with > its design in the first place. > > Therefore, it is better IMO to verify whether the K8_NB thing is > initialized properly and exit early if not. And this is what the two > patches I'm suggesting try to accomplish: > > 1. Call into K8_NB on AMD unconditionally so that we have those > initialized (we'll be needing them more in the future, I guess) > > 2. Check in the L3 cache code whether K8_NB has initted properly and > bail out if not. > > With this, no need for NULL ptr checks anywhere and you got > better/smaller/more readable code with the proper assumptions in place. Well, we have #ifdef CONFIG_K8_NB static inline struct pci_dev *node_to_k8_nb_misc(int node) { return (node < num_k8_northbridges) ? k8_northbridges[node] : NULL; } #else static inline struct pci_dev *node_to_k8_nb_misc(int node) { return NULL; } #endif So it legitimately returns NULL in two cases: 1) if someone passes too large node 2) if CONFIG_K8_NB is unset 1) Either we assume that node will always be in the range (i.e. amd_get_nb_id() will never go crazy return anything bogus), and then we could just drop the test completely. Or we want to check for such the possibility, and then node_to_k8_nb_misc() is going to return NULL in such cases, and so we want to check for it. 2) is now moot, as all three in-tree callers are now under proper ifdefs (CONFIG_CPU_SUP_AMD, which depends on CONFIG_K8_NB after your patch has been applied). So I believe we could remove it, right? Either way, current state seems inconsistent. So either we should add the return value check to amd_calc_l3_indices() as well, or remove all the NULL magic altogether, i.e. the (untested) patch below. What do you think? From: Jiri Kosina <jkosina@...e.cz> Subject: x86, cacheinfo: remove unnecessary NULL pointer checks As K8_NB thing is now always initialized on AMD CPUs, and we don't have any caller that would be outside CONFIG_CPU_SUP_AMD/CONFIG_K8_NB, it is safe to assume that kb_northbridges has been always initialized). Signed-off-by: Jiri Kosina <jkosina@...e.cz> arch/x86/include/asm/k8.h | 10 +--------- arch/x86/kernel/cpu/intel_cacheinfo.c | 6 ------ 2 files changed, 1 insertions(+), 15 deletions(-) diff --git a/arch/x86/include/asm/k8.h b/arch/x86/include/asm/k8.h index f70e600..0cff8ba 100644 --- a/arch/x86/include/asm/k8.h +++ b/arch/x86/include/asm/k8.h @@ -15,17 +15,9 @@ extern int k8_get_nodes(struct bootnode *nodes); extern int k8_numa_init(unsigned long start_pfn, unsigned long end_pfn); extern int k8_scan_nodes(void); -#ifdef CONFIG_K8_NB static inline struct pci_dev *node_to_k8_nb_misc(int node) { - return (node < num_k8_northbridges) ? k8_northbridges[node] : NULL; + return k8_northbridges[node]; } -#else -static inline struct pci_dev *node_to_k8_nb_misc(int node) -{ - return NULL; -} -#endif - #endif /* _ASM_X86_K8_H */ diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c index b3eeb66..6c5f3cd 100644 --- a/arch/x86/kernel/cpu/intel_cacheinfo.c +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c @@ -355,9 +355,6 @@ static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf, if (!this_leaf->can_disable) return -EINVAL; - if (!dev) - return -EINVAL; - pci_read_config_dword(dev, 0x1BC + index * 4, ®); return sprintf(buf, "0x%08x\n", reg); } @@ -388,9 +385,6 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf, if (!capable(CAP_SYS_ADMIN)) return -EPERM; - if (!dev) - return -EINVAL; - if (strict_strtoul(buf, 10, &val) < 0) return -EINVAL; -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists