| Message ID | YIE7pdqV/h10tEAK@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scpi: prevent ternary sign expansion bug | expand |
Hi, On Thu, Apr 22, 2021 at 12:02:29PM +0300, Dan Carpenter wrote: > How type promotion works in ternary expressions is a bit tricky. > The problem is that scpi_clk_get_val() returns longs, "ret" is a int > which holds a negative error code, and le32_to_cpu() is an unsigned int. > We want the negative error code to be cast to a negative long. But > because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and > becomes a high positive and then it is promoted to long and it is still > a high positive value. > > Fix this by getting rid of the ternary. I wonder how/if the callers up in the stack check/expect ever effectively for a 2-complement negative value inside the returned unsigned long...given that this plugs finally into CLK framework struct clk_ops.recalc_rate via clk-scpi.c which also expects unsigned long....but that's another story. FWIW regarding this patch: Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> Thanks Cristian > > Fixes: 8cb7cf56c9fe ("firmware: add support for ARM System Control and Power Interface(SCPI) protocol") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/firmware/arm_scpi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index d0dee37ad522..3bf61854121d 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -552,8 +552,10 @@ static unsigned long scpi_clk_get_val(u16 clk_id) > > ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, > sizeof(le_clk_id), &rate, sizeof(rate)); > + if (ret) > + return ret; > > - return ret ? ret : le32_to_cpu(rate); > + return le32_to_cpu(rate); > } > > static int scpi_clk_set_val(u16 clk_id, unsigned long rate) > -- > 2.30.2 >
On Thu, Apr 22, 2021 at 11:17:09AM +0100, Cristian Marussi wrote: > Hi, > > On Thu, Apr 22, 2021 at 12:02:29PM +0300, Dan Carpenter wrote: > > How type promotion works in ternary expressions is a bit tricky. > > The problem is that scpi_clk_get_val() returns longs, "ret" is a int > > which holds a negative error code, and le32_to_cpu() is an unsigned int. > > We want the negative error code to be cast to a negative long. But > > because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and > > becomes a high positive and then it is promoted to long and it is still > > a high positive value. > > > > Fix this by getting rid of the ternary. > > I wonder how/if the callers up in the stack check/expect ever effectively for a > 2-complement negative value inside the returned unsigned long...given that this > plugs finally into CLK framework struct clk_ops.recalc_rate via clk-scpi.c which > also expects unsigned long....but that's another story. > > FWIW regarding this patch: > > Reviewed-by: Cristian Marussi <cristian.marussi@arm.com> > > Thanks > > Cristian @Sudeep, as a second though, looking at .recalc_rate() definition inside include/linux/clk-provider.h:struct clk_ops which is the direct caller of this SCPI clk function, I wonder if, instead, on error we should not return here just ZERO as the returned clock rate value as in: if (ret) return 0; given that the error code returned inside the unsigned long won't be ever considerd as such apparently, so not sure if it'd be worst to return a very big fake value or zero... Thanks Cristian > > > > Fixes: 8cb7cf56c9fe ("firmware: add support for ARM System Control and Power Interface(SCPI) protocol") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/firmware/arm_scpi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > > index d0dee37ad522..3bf61854121d 100644 > > --- a/drivers/firmware/arm_scpi.c > > +++ b/drivers/firmware/arm_scpi.c > > @@ -552,8 +552,10 @@ static unsigned long scpi_clk_get_val(u16 clk_id) > > > > ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, > > sizeof(le_clk_id), &rate, sizeof(rate)); > > + if (ret) > > + return ret; > > > > - return ret ? ret : le32_to_cpu(rate); > > + return le32_to_cpu(rate); > > } > > > > static int scpi_clk_set_val(u16 clk_id, unsigned long rate) > > -- > > 2.30.2 > >
(dropping Tixy as I am sure it will bounce, he left/retired from Linaro long back) On Thu, Apr 22, 2021 at 12:02:29PM +0300, Dan Carpenter wrote: > How type promotion works in ternary expressions is a bit tricky. > The problem is that scpi_clk_get_val() returns longs, "ret" is a int > which holds a negative error code, and le32_to_cpu() is an unsigned int. Agreed. > We want the negative error code to be cast to a negative long. But > because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and > becomes a high positive and then it is promoted to long and it is still > a high positive value. > Thanks a lot for finding and fixing the bug! > Fix this by getting rid of the ternary. > > Fixes: 8cb7cf56c9fe ("firmware: add support for ARM System Control and Power Interface(SCPI) protocol") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/firmware/arm_scpi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index d0dee37ad522..3bf61854121d 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -552,8 +552,10 @@ static unsigned long scpi_clk_get_val(u16 clk_id) > > ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, > sizeof(le_clk_id), &rate, sizeof(rate)); > + if (ret) > + return ret; This could be still an issue, ideally I would prefer to pass the return value via argument pointer and always return success/failure as return value. Can't remember any reason for this. Since this is old interface with limited platform to test, I think returning 0 as clock rate on error should be fine as Cristain suggested. If you agree with that, I can fix up when applying. If you don't, we can look at changing the scpi interface to clock driver which will anyway need to do the same(i.e. send 0 in case of error) Let me know. -- Regards, Sudeep
On Thu, Apr 22, 2021 at 06:46:31PM +0100, Sudeep Holla wrote: > (dropping Tixy as I am sure it will bounce, he left/retired from Linaro > long back) > > On Thu, Apr 22, 2021 at 12:02:29PM +0300, Dan Carpenter wrote: > > How type promotion works in ternary expressions is a bit tricky. > > The problem is that scpi_clk_get_val() returns longs, "ret" is a int > > which holds a negative error code, and le32_to_cpu() is an unsigned int. > > Agreed. > > > We want the negative error code to be cast to a negative long. But > > because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and > > becomes a high positive and then it is promoted to long and it is still > > a high positive value. > > > > Thanks a lot for finding and fixing the bug! > > > Fix this by getting rid of the ternary. > > > > Fixes: 8cb7cf56c9fe ("firmware: add support for ARM System Control and Power Interface(SCPI) protocol") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/firmware/arm_scpi.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > > index d0dee37ad522..3bf61854121d 100644 > > --- a/drivers/firmware/arm_scpi.c > > +++ b/drivers/firmware/arm_scpi.c > > @@ -552,8 +552,10 @@ static unsigned long scpi_clk_get_val(u16 clk_id) > > > > ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, > > sizeof(le_clk_id), &rate, sizeof(rate)); > > + if (ret) > > + return ret; > > This could be still an issue, ideally I would prefer to pass the return > value via argument pointer and always return success/failure as return > value. Can't remember any reason for this. Since this is old interface > with limited platform to test, I think returning 0 as clock rate on error > should be fine as Cristain suggested. If you agree with that, I can > fix up when applying. > That sounds great. Thanks. regards, dan carpenter
On Thu, 22 Apr 2021 12:02:29 +0300, Dan Carpenter wrote: > How type promotion works in ternary expressions is a bit tricky. > The problem is that scpi_clk_get_val() returns longs, "ret" is a int > which holds a negative error code, and le32_to_cpu() is an unsigned int. > We want the negative error code to be cast to a negative long. But > because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and > becomes a high positive and then it is promoted to long and it is still > a high positive value. > > [...] Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/1] firmware: arm_scpi: prevent ternary sign expansion bug https://git.kernel.org/sudeep.holla/c/d9cd78edb2 -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index d0dee37ad522..3bf61854121d 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -552,8 +552,10 @@ static unsigned long scpi_clk_get_val(u16 clk_id) ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, sizeof(le_clk_id), &rate, sizeof(rate)); + if (ret) + return ret; - return ret ? ret : le32_to_cpu(rate); + return le32_to_cpu(rate); } static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
How type promotion works in ternary expressions is a bit tricky. The problem is that scpi_clk_get_val() returns longs, "ret" is a int which holds a negative error code, and le32_to_cpu() is an unsigned int. We want the negative error code to be cast to a negative long. But because le32_to_cpu() is an u32 then "ret" is type promoted to u32 and becomes a high positive and then it is promoted to long and it is still a high positive value. Fix this by getting rid of the ternary. Fixes: 8cb7cf56c9fe ("firmware: add support for ARM System Control and Power Interface(SCPI) protocol") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/firmware/arm_scpi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)