Hi Michal, > -----Original Message----- > From: Michal Simek <[email protected]> > Sent: 27 October 2020 02:53 > To: Arnd Bergmann <[email protected]>; Michal Simek <[email protected]>; > Rajan Vaja <[email protected]> > Cc: Arnd Bergmann <[email protected]>; Rajan Vaja <[email protected]>; Greg > Kroah-Hartman <[email protected]>; Tejas Patel > <[email protected]>; [email protected]; linux- > [email protected]; git <[email protected]> > Subject: Re: [PATCH] firmware: xilinx: fix out-of-bounds access > > > > On 26. 10. 20 16:54, Arnd Bergmann wrote: > > From: Arnd Bergmann <[email protected]> > > > > The zynqmp_pm_set_suspend_mode() and > zynqmp_pm_get_trustzone_version() > > functions pass values as api_id into zynqmp_pm_invoke_fn > > that are beyond PM_API_MAX, resulting in an out-of-bounds access: > > > > drivers/firmware/xilinx/zynqmp.c: In function > 'zynqmp_pm_set_suspend_mode': > > drivers/firmware/xilinx/zynqmp.c:150:24: warning: array subscript 2562 is > > above > array bounds of 'u32[64]' {aka 'unsigned int[64]'} [-Warray-bounds] > > 150 | if (zynqmp_pm_features[api_id] != PM_FEATURE_UNCHECKED) > > | ~~~~~~~~~~~~~~~~~~^~~~~~~~ > > drivers/firmware/xilinx/zynqmp.c:28:12: note: while referencing > 'zynqmp_pm_features' > > 28 | static u32 zynqmp_pm_features[PM_API_MAX]; > > | ^~~~~~~~~~~~~~~~~~ > > Which CONFIG option/tool is reporting this issue? > > > > > Replace the resulting undefined behavior with an error return. > > This may break some things that happen to work at the moment > > but seems better than randomly overwriting kernel data. > > > > I assume we need additional fixes for the two functions that now > > return an error. > > > > Fixes: 76582671eb5d ("firmware: xilinx: Add Zynqmp firmware driver") > > Fixes: e178df31cf41 ("firmware: xilinx: Implement ZynqMP power management > APIs") > > Signed-off-by: Arnd Bergmann <[email protected]> > > --- > > drivers/firmware/xilinx/zynqmp.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/firmware/xilinx/zynqmp.c > b/drivers/firmware/xilinx/zynqmp.c > > index 8d1ff2454e2e..efb8a66efc68 100644 > > --- a/drivers/firmware/xilinx/zynqmp.c > > +++ b/drivers/firmware/xilinx/zynqmp.c > > @@ -147,6 +147,9 @@ static int zynqmp_pm_feature(u32 api_id) > > return 0; > > > > /* Return value if feature is already checked */ > > + if (api_id > ARRAY_SIZE(zynqmp_pm_features)) > > + return PM_FEATURE_INVALID; > > + > > if (zynqmp_pm_features[api_id] != PM_FEATURE_UNCHECKED) > > return zynqmp_pm_features[api_id]; > > > > > > Definitely good catch but not quite sure what should be correct reaction. > Rajan: Can you please take a look at it with priority? [Rajan] Change looks fine to me.
Thanks, Rajan > > Thanks, > Michal
