Messages in this thread | | | From | Rajan Vaja <> | Subject | RE: [PATCH] firmware: xilinx: fix out-of-bounds access | Date | Tue, 27 Oct 2020 12:23:24 +0000 |
| |
Hi Michal,
> -----Original Message----- > From: Michal Simek <michal.simek@xilinx.com> > Sent: 27 October 2020 02:53 > To: Arnd Bergmann <arnd@kernel.org>; Michal Simek <michals@xilinx.com>; > Rajan Vaja <RAJANV@xilinx.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Rajan Vaja <RAJANV@xilinx.com>; Greg > Kroah-Hartman <gregkh@linuxfoundation.org>; Tejas Patel > <TEJASP@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; git <git@xilinx.com> > Subject: Re: [PATCH] firmware: xilinx: fix out-of-bounds access > > > > 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? [Rajan] Change looks fine to me.
Thanks, Rajan > > Thanks, > Michal
| |