Re: [Xen-devel] [PATCH V5] xen: arm: platforms: Adding reset support for xgene arm64 platform.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-01-27 at 14:12 +0530, Pranavkumar Sawargaonkar wrote:
> +    /* Retrieve base address and size */
> +    res = dt_device_get_address(dev, 0, &reset_addr, &reset_size);
> +    if ( res )
> +    {
> +        printk("XGENE: Unable to retrieve the base address for reset\n");
> +        return 0;
> +    }
> +
> +    /* Get reset mask */
> +    res = dt_property_read_u32(dev, "mask", &reset_mask);
> +    if ( !res )
> +    {

At this point reset_addr and reset_size are set and xgene_storm_reset
will try to use them with reset_mask -- which I suppose is 0 at this
point (due to .bss initialisation + dt_prop_read not touching it on
failure).

Is that safe / ok?

In fact, on failure of the dt_device_get_address xgene_storm_reset will
try and use the values too and they may be uninitialised or may be
DT_BAD_ADDR depending on the location of the failure.

Could this be potentially harmful? Obviously it is not expected to
successfully reset under these circumstances, but what else could it do?
(e.g. turn off the fan and melt the system?)

I'd suggest to set a flag right at the end which xgene_storm_reset can
check. Either an explicit boolean or initialise reset_addr to ~(u64)0
when it is declared and gather the address into a local variable before
setting the global only after init has succeeded.

(I'd also accept your assurances that writing to random memory locations
is safe, on the off chance that this is true ;-))

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel