[PATCH] firmware: arm_scmi: Fix possible scmi_linux_errmap buffer overflow

[Sudeep Holla]

On Wed, Jul 07, 2021 at 03:06:25PM +0100, Cristian Marussi wrote:
> On Wed, Jul 07, 2021 at 02:50:28PM +0100, Sudeep Holla wrote:
> > The scmi_linux_errmap buffer access index is supposed to depend on the
> > array size to prevent element out of bounds access. It uses SCMI_ERR_MAX
> > to check bounds but that can mismatch with the array size. It also
> > changes the success into -EIO though scmi_linux_errmap is never used in
> > case of success, it is expected to work for success case too.
> > 
> > It is slightly confusing code as the negative of the error code
> > is used as index to the buffer. Fix it by negating it at the start and
> > make it more readable.
> > 
> > Reported-by: kernel test robot <lkp at intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter at oracle.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla at arm.com>
> > ---
> >  drivers/firmware/arm_scmi/driver.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > (Based on https://lore.kernel.org/r/20210707134739.1869481-1-sudeep.holla@arm.com)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 66e5e694be7d..2a5c1b3658c4 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -166,8 +166,10 @@ static const int scmi_linux_errmap[] = {
> >  
> >  static inline int scmi_to_linux_errno(int errno)
> >  {
> > -	if (errno < SCMI_SUCCESS && errno > SCMI_ERR_MAX)
> > -		return scmi_linux_errmap[-errno];
> > +	int err_idx = -errno;
> > +
> > +	if (err_idx >= SCMI_SUCCESS && err_idx < ARRAY_SIZE(scmi_linux_errmap))
> > +		return scmi_linux_errmap[err_idx];
> >  	return -EIO;
> >  }
> >  
> Hi,
> 
> Looks good to me; now SCMI_ERR_MAX is not referenced anymore by anyone
> but I suppose is good practice to still keep it as an end-marker for
> scmi_error_codes enum.
>

Good point, I will drop it as there are no users. It can be added later
if needed.

> Reviewed-by: Cristian Marussi <cristian.marussi at arm.com>
>

Thanks !

-- 
Regards,
Sudeep