linux-kernel - Re: [PATCH v3] ARM: pxa: sharpsl_param.c: fix invalid memory access

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]

Message-ID: <20130918233652.GY12758@n2100.arm.linux.org.uk>
Date:	Thu, 19 Sep 2013 00:36:52 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Andrea Adami <andrea.adami@...il.com>
Cc:	linux-arm-kernel@...ts.infradead.org, arm@...nel.org,
	Olof Johansson <olof@...om.net>,
	Marko Katic <dromede@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ARM: pxa: sharpsl_param.c: fix invalid memory access

On Thu, Sep 19, 2013 at 01:18:39AM +0200, Andrea Adami wrote:
> After commit 72662e01088394577be4a3f14da94cf87bea2591
> ARM: head.S: only include __turn_mmu_on in the initial identity mapping
> 
> Zaurus PXA devices call sharpsl_save_param() during fixup and hang on
> boot because memcpy refers to physical addresses no longer valid if the
> MMU is setup.
> Zaurus collie (SA1100) is unaffected (function is called in init_machine).

Yes, moving that will have broken this, but that's only because this code
is making assumptions about stuff which we don't really "guarantee" will
be the case.  For PXA, moving it to use the virtual address is the right
solution, one which would've worked before the change too.

For SA11x0 - I suspect 0xe8ffc000 is a virtual address already (it
points into the main flash) so I think this is okay.

Some comments though:

> diff --git a/arch/arm/common/sharpsl_param.c b/arch/arm/common/sharpsl_param.c
> index d56c932..4dcd349 100644
> --- a/arch/arm/common/sharpsl_param.c
> +++ b/arch/arm/common/sharpsl_param.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/string.h>
>  #include <asm/mach/sharpsl_param.h>
> +#include <asm/memory.h>
>  
>  /*
>   * Certain hardware parameters determined at the time of device manufacture,
> @@ -23,6 +24,8 @@
>   * them early in the boot process, then pass them to the appropriate drivers.
>   * Not all devices use all parameters but the format is common to all.
>   */
> +
> +

Please be more careful about adding extraneous blank lines/spaces to files.

>  #ifdef CONFIG_ARCH_SA1100
>  #define PARAM_BASE	0xe8ffc000
>  #else
> @@ -41,7 +44,17 @@ EXPORT_SYMBOL(sharpsl_param);
>  
>  void sharpsl_save_param(void)
>  {
> -	memcpy(&sharpsl_param, (void *)PARAM_BASE, sizeof(struct sharpsl_param_info));
> +/* NOTE:
> + * Zaurus PXA devices call sharpsl_save_param() during fixup and the MMU
> + * is setup so we need to translate the physical address.
> + * Zaurus collie (SA1100) is unaffected (function is called in init_machine).
> + */
> +#ifdef CONFIG_ARCH_SA1100
> +	void *param_start = (void *)PARAM_BASE;
> +#else
> +	void *param_start = phys_to_virt(PARAM_BASE);
> +#endif
> +	memcpy(&sharpsl_param, param_start, sizeof(struct sharpsl_param_info));

I'd suggest moving the (void *) cast to the #defines above, and also the
phys_to_virt() too.  With a reasonable commit log, I don't think we need
the comments either.

> @@ -58,5 +71,3 @@ void sharpsl_save_param(void)
>  	if (sharpsl_param.adadj_keyword != AD_MAGIC)
>  		sharpsl_param.adadj=-1;
>  }
> -
> -

While removal of lines is a good thing, this isn't really part of this
patch, but I'd say in this case don't worry about that.

Thanks.
--
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/