Messages in this thread | | | Subject | Re: [PATCH] firmware: xilinx: fix out-of-bounds access | From | Michal Simek <> | Date | Tue, 27 Oct 2020 10:53:11 +0100 |
| |
On 26. 10. 20 16:54, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > 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 <arnd@arndb.de> > --- > 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?
Thanks, Michal
| |