media: dvb-frontends: avoid stack overflow warnings with clang
Commit Message
From: Arnd Bergmann <arnd@arndb.de>
A previous patch worked around a KASAN issue in stv0367, now a similar
problem showed up with clang:
drivers/media/dvb-frontends/stv0367.c:1222:12: error: stack frame size (3624) exceeds limit (2048) in 'stv0367ter_set_frontend' [-Werror,-Wframe-larger-than]
1214 | static int stv0367ter_set_frontend(struct dvb_frontend *fe)
Rework the stv0367_writereg() function to be simpler and mark both
register access functions as noinline_for_stack so the temporary
i2c_msg structures do not get duplicated on the stack when KASAN_STACK
is enabled.
Fixes: 3cd890dbe2a4 ("media: dvb-frontends: fix i2c access helpers for KASAN")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/media/dvb-frontends/stv0367.c | 34 +++++++--------------------
1 file changed, 8 insertions(+), 26 deletions(-)
Comments
Hi,
On Fri, Feb 16, 2024 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> A previous patch worked around a KASAN issue in stv0367, now a similar
> problem showed up with clang:
>
> drivers/media/dvb-frontends/stv0367.c:1222:12: error: stack frame size (3624) exceeds limit (2048) in 'stv0367ter_set_frontend' [-Werror,-Wframe-larger-than]
> 1214 | static int stv0367ter_set_frontend(struct dvb_frontend *fe)
>
> Rework the stv0367_writereg() function to be simpler and mark both
> register access functions as noinline_for_stack so the temporary
> i2c_msg structures do not get duplicated on the stack when KASAN_STACK
> is enabled.
>
> Fixes: 3cd890dbe2a4 ("media: dvb-frontends: fix i2c access helpers for KASAN")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/dvb-frontends/stv0367.c | 34 +++++++--------------------
> 1 file changed, 8 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/stv0367.c b/drivers/media/dvb-frontends/stv0367.c
> index 48326434488c..72540ef4e5f8 100644
> --- a/drivers/media/dvb-frontends/stv0367.c
> +++ b/drivers/media/dvb-frontends/stv0367.c
> @@ -118,50 +118,32 @@ static const s32 stv0367cab_RF_LookUp2[RF_LOOKUP_TABLE2_SIZE][RF_LOOKUP_TABLE2_S
> }
> };
>
> -static
> -int stv0367_writeregs(struct stv0367_state *state, u16 reg, u8 *data, int len)
> +static noinline_for_stack
> +int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
> {
> - u8 buf[MAX_XFER_SIZE];
> + u8 buf[3] = { MSB(reg), LSB(reg), data };
> struct i2c_msg msg = {
> .addr = state->config->demod_address,
> .flags = 0,
> .buf = buf,
> - .len = len + 2
> + .len = 3,
> };
> int ret;
>
> - if (2 + len > sizeof(buf)) {
> - printk(KERN_WARNING
> - "%s: i2c wr reg=%04x: len=%d is too big!\n",
> - KBUILD_MODNAME, reg, len);
> - return -EINVAL;
> - }
> -
> -
> - buf[0] = MSB(reg);
> - buf[1] = LSB(reg);
> - memcpy(buf + 2, data, len);
I'm curious why a copy was made at all.
Reviewed-by: Justin Stitt <justinstitt@google.com>
> -
> if (i2cdebug)
> printk(KERN_DEBUG "%s: [%02x] %02x: %02x\n", __func__,
> - state->config->demod_address, reg, buf[2]);
> + state->config->demod_address, reg, data);
>
> ret = i2c_transfer(state->i2c, &msg, 1);
> if (ret != 1)
> printk(KERN_ERR "%s: i2c write error! ([%02x] %02x: %02x)\n",
> - __func__, state->config->demod_address, reg, buf[2]);
> + __func__, state->config->demod_address, reg, data);
>
> return (ret != 1) ? -EREMOTEIO : 0;
> }
>
> -static int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
> -{
> - u8 tmp = data; /* see gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */
> -
> - return stv0367_writeregs(state, reg, &tmp, 1);
> -}
> -
> -static u8 stv0367_readreg(struct stv0367_state *state, u16 reg)
> +static noinline_for_stack
> +u8 stv0367_readreg(struct stv0367_state *state, u16 reg)
> {
> u8 b0[] = { 0, 0 };
> u8 b1[] = { 0 };
> --
> 2.39.2
>
Thanks
Justin
On Fri, Feb 16, 2024, at 19:56, Justin Stitt wrote:
> On Fri, Feb 16, 2024 at 8:32 AM Arnd Bergmann <arnd@kernel.org> wrote:
>> -
>> - buf[0] = MSB(reg);
>> - buf[1] = LSB(reg);
>> - memcpy(buf + 2, data, len);
>
> I'm curious why a copy was made at all.
>
> Reviewed-by: Justin Stitt <justinstitt@google.com>
I guess the idea was to allow writing arbitrarily long
contents to a single register with a single decriptor.
The current version of the driver only ever writes a single
byte value, but maybe it either used to have other writes,
or it was meant to support them later but never did.
Arnd
@@ -118,50 +118,32 @@ static const s32 stv0367cab_RF_LookUp2[RF_LOOKUP_TABLE2_SIZE][RF_LOOKUP_TABLE2_S
}
};
-static
-int stv0367_writeregs(struct stv0367_state *state, u16 reg, u8 *data, int len)
+static noinline_for_stack
+int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
{
- u8 buf[MAX_XFER_SIZE];
+ u8 buf[3] = { MSB(reg), LSB(reg), data };
struct i2c_msg msg = {
.addr = state->config->demod_address,
.flags = 0,
.buf = buf,
- .len = len + 2
+ .len = 3,
};
int ret;
- if (2 + len > sizeof(buf)) {
- printk(KERN_WARNING
- "%s: i2c wr reg=%04x: len=%d is too big!\n",
- KBUILD_MODNAME, reg, len);
- return -EINVAL;
- }
-
-
- buf[0] = MSB(reg);
- buf[1] = LSB(reg);
- memcpy(buf + 2, data, len);
-
if (i2cdebug)
printk(KERN_DEBUG "%s: [%02x] %02x: %02x\n", __func__,
- state->config->demod_address, reg, buf[2]);
+ state->config->demod_address, reg, data);
ret = i2c_transfer(state->i2c, &msg, 1);
if (ret != 1)
printk(KERN_ERR "%s: i2c write error! ([%02x] %02x: %02x)\n",
- __func__, state->config->demod_address, reg, buf[2]);
+ __func__, state->config->demod_address, reg, data);
return (ret != 1) ? -EREMOTEIO : 0;
}
-static int stv0367_writereg(struct stv0367_state *state, u16 reg, u8 data)
-{
- u8 tmp = data; /* see gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 */
-
- return stv0367_writeregs(state, reg, &tmp, 1);
-}
-
-static u8 stv0367_readreg(struct stv0367_state *state, u16 reg)
+static noinline_for_stack
+u8 stv0367_readreg(struct stv0367_state *state, u16 reg)
{
u8 b0[] = { 0, 0 };
u8 b1[] = { 0 };