From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.kapsi.fi ([217.30.184.167]:36992 "EHLO mail.kapsi.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750939Ab3KGSzU (ORCPT ); Thu, 7 Nov 2013 13:55:20 -0500 Message-ID: <527BE215.4080702@iki.fi> Date: Thu, 07 Nov 2013 20:55:17 +0200 From: Antti Palosaari MIME-Version: 1.0 To: Mauro Carvalho Chehab , unlisted-recipients:; CC: Linux Media Mailing List , Mauro Carvalho Chehab Subject: Re: [PATCH v3 18/29] [media] tuners: Don't use dynamic static allocation References: <1383645702-30636-1-git-send-email-m.chehab@samsung.com> <1383645702-30636-19-git-send-email-m.chehab@samsung.com> <527926CB.8070006@iki.fi> In-Reply-To: <527926CB.8070006@iki.fi> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Mauro, I just notified these are all broken. The reason is here that I2C adapter sets I2C operation length using sizeof(buf). Please take a look of all there patches and check existing use of sizeof(buf). regards Antti On 05.11.2013 19:11, Antti Palosaari wrote: > Acked-by: Antti Palosaari > Reviewed-by: Antti Palosaari > > Antti > > On 05.11.2013 12:01, Mauro Carvalho Chehab wrote: >> Dynamic static allocation is evil, as Kernel stack is too low, and >> compilation complains about it on some archs: >> drivers/media/tuners/e4000.c:50:1: warning: 'e4000_wr_regs' uses >> dynamic stack allocation [enabled by default] >> drivers/media/tuners/e4000.c:83:1: warning: 'e4000_rd_regs' uses >> dynamic stack allocation [enabled by default] >> drivers/media/tuners/fc2580.c:66:1: warning: >> 'fc2580_wr_regs.constprop.1' uses dynamic stack allocation [enabled by >> default] >> drivers/media/tuners/fc2580.c:98:1: warning: >> 'fc2580_rd_regs.constprop.0' uses dynamic stack allocation [enabled by >> default] >> drivers/media/tuners/tda18212.c:57:1: warning: 'tda18212_wr_regs' >> uses dynamic stack allocation [enabled by default] >> drivers/media/tuners/tda18212.c:90:1: warning: >> 'tda18212_rd_regs.constprop.0' uses dynamic stack allocation [enabled >> by default] >> drivers/media/tuners/tda18218.c:60:1: warning: 'tda18218_wr_regs' >> uses dynamic stack allocation [enabled by default] >> drivers/media/tuners/tda18218.c:92:1: warning: >> 'tda18218_rd_regs.constprop.0' uses dynamic stack allocation [enabled >> by default] >> >> Instead, let's enforce a limit for the buffer. Considering that I2C >> transfers are generally limited, and that devices used on USB has a >> max data length of 64 bytes for the control URBs. >> >> So, it seem safe to use 64 bytes as the hard limit for all those devices. >> >> On most cases, the limit is a way lower than that, but this limit >> is small enough to not affect the Kernel stack, and it is a no brain >> limit, as using smaller ones would require to either carefully each >> driver or to take a look on each datasheet. >> >> Signed-off-by: Mauro Carvalho Chehab >> --- >> drivers/media/tuners/e4000.c | 21 +++++++++++++++++++-- >> drivers/media/tuners/fc2580.c | 21 +++++++++++++++++++-- >> drivers/media/tuners/tda18212.c | 21 +++++++++++++++++++-- >> drivers/media/tuners/tda18218.c | 21 +++++++++++++++++++-- >> 4 files changed, 76 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >> index ad9309da4a91..30192463c9e1 100644 >> --- a/drivers/media/tuners/e4000.c >> +++ b/drivers/media/tuners/e4000.c >> @@ -20,11 +20,14 @@ >> >> #include "e4000_priv.h" >> >> +/* Max transfer size done by I2C transfer functions */ >> +#define MAX_XFER_SIZE 64 >> + >> /* write multiple registers */ >> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[1 + len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[1] = { >> { >> .addr = priv->cfg->i2c_addr, >> @@ -34,6 +37,13 @@ static int e4000_wr_regs(struct e4000_priv *priv, >> u8 reg, u8 *val, int len) >> } >> }; >> >> + if (1 + len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c wr reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> buf[0] = reg; >> memcpy(&buf[1], val, len); >> >> @@ -53,7 +63,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 >> reg, u8 *val, int len) >> static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[2] = { >> { >> .addr = priv->cfg->i2c_addr, >> @@ -68,6 +78,13 @@ static int e4000_rd_regs(struct e4000_priv *priv, >> u8 reg, u8 *val, int len) >> } >> }; >> >> + if (len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c rd reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> ret = i2c_transfer(priv->i2c, msg, 2); >> if (ret == 2) { >> memcpy(val, buf, len); >> diff --git a/drivers/media/tuners/fc2580.c >> b/drivers/media/tuners/fc2580.c >> index 81f38aae9c66..430fa5163ec7 100644 >> --- a/drivers/media/tuners/fc2580.c >> +++ b/drivers/media/tuners/fc2580.c >> @@ -20,6 +20,9 @@ >> >> #include "fc2580_priv.h" >> >> +/* Max transfer size done by I2C transfer functions */ >> +#define MAX_XFER_SIZE 64 >> + >> /* >> * TODO: >> * I2C write and read works only for one single register. Multiple >> registers >> @@ -41,7 +44,7 @@ >> static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[1 + len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[1] = { >> { >> .addr = priv->cfg->i2c_addr, >> @@ -51,6 +54,13 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, >> u8 reg, u8 *val, int len) >> } >> }; >> >> + if (1 + len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c wr reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> buf[0] = reg; >> memcpy(&buf[1], val, len); >> >> @@ -69,7 +79,7 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, >> u8 reg, u8 *val, int len) >> static int fc2580_rd_regs(struct fc2580_priv *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[2] = { >> { >> .addr = priv->cfg->i2c_addr, >> @@ -84,6 +94,13 @@ static int fc2580_rd_regs(struct fc2580_priv *priv, >> u8 reg, u8 *val, int len) >> } >> }; >> >> + if (len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c rd reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> ret = i2c_transfer(priv->i2c, msg, 2); >> if (ret == 2) { >> memcpy(val, buf, len); >> diff --git a/drivers/media/tuners/tda18212.c >> b/drivers/media/tuners/tda18212.c >> index e4a84ee231cf..b3a4adf9ff8f 100644 >> --- a/drivers/media/tuners/tda18212.c >> +++ b/drivers/media/tuners/tda18212.c >> @@ -20,6 +20,9 @@ >> >> #include "tda18212.h" >> >> +/* Max transfer size done by I2C transfer functions */ >> +#define MAX_XFER_SIZE 64 >> + >> struct tda18212_priv { >> struct tda18212_config *cfg; >> struct i2c_adapter *i2c; >> @@ -32,7 +35,7 @@ static int tda18212_wr_regs(struct tda18212_priv >> *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[len+1]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[1] = { >> { >> .addr = priv->cfg->i2c_address, >> @@ -42,6 +45,13 @@ static int tda18212_wr_regs(struct tda18212_priv >> *priv, u8 reg, u8 *val, >> } >> }; >> >> + if (1 + len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c wr reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> buf[0] = reg; >> memcpy(&buf[1], val, len); >> >> @@ -61,7 +71,7 @@ static int tda18212_rd_regs(struct tda18212_priv >> *priv, u8 reg, u8 *val, >> int len) >> { >> int ret; >> - u8 buf[len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[2] = { >> { >> .addr = priv->cfg->i2c_address, >> @@ -76,6 +86,13 @@ static int tda18212_rd_regs(struct tda18212_priv >> *priv, u8 reg, u8 *val, >> } >> }; >> >> + if (len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c rd reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> ret = i2c_transfer(priv->i2c, msg, 2); >> if (ret == 2) { >> memcpy(val, buf, len); >> diff --git a/drivers/media/tuners/tda18218.c >> b/drivers/media/tuners/tda18218.c >> index 2d31aeb6b088..7e2b32ee5349 100644 >> --- a/drivers/media/tuners/tda18218.c >> +++ b/drivers/media/tuners/tda18218.c >> @@ -20,11 +20,14 @@ >> >> #include "tda18218_priv.h" >> >> +/* Max transfer size done by I2C transfer functions */ >> +#define MAX_XFER_SIZE 64 >> + >> /* write multiple registers */ >> static int tda18218_wr_regs(struct tda18218_priv *priv, u8 reg, u8 >> *val, u8 len) >> { >> int ret = 0, len2, remaining; >> - u8 buf[1 + len]; >> + u8 buf[MAX_XFER_SIZE]; >> struct i2c_msg msg[1] = { >> { >> .addr = priv->cfg->i2c_address, >> @@ -33,6 +36,13 @@ static int tda18218_wr_regs(struct tda18218_priv >> *priv, u8 reg, u8 *val, u8 len) >> } >> }; >> >> + if (1 + len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c wr reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> for (remaining = len; remaining > 0; >> remaining -= (priv->cfg->i2c_wr_max - 1)) { >> len2 = remaining; >> @@ -63,7 +73,7 @@ static int tda18218_wr_regs(struct tda18218_priv >> *priv, u8 reg, u8 *val, u8 len) >> static int tda18218_rd_regs(struct tda18218_priv *priv, u8 reg, u8 >> *val, u8 len) >> { >> int ret; >> - u8 buf[reg+len]; /* we must start read always from reg 0x00 */ >> + u8 buf[MAX_XFER_SIZE]; /* we must start read always from reg 0x00 */ >> struct i2c_msg msg[2] = { >> { >> .addr = priv->cfg->i2c_address, >> @@ -78,6 +88,13 @@ static int tda18218_rd_regs(struct tda18218_priv >> *priv, u8 reg, u8 *val, u8 len) >> } >> }; >> >> + if (reg + len > sizeof(buf)) { >> + dev_warn(&priv->i2c->dev, >> + "%s: i2c wr reg=%04x: len=%d is too big!\n", >> + KBUILD_MODNAME, reg, len); >> + return -EINVAL; >> + } >> + >> ret = i2c_transfer(priv->i2c, msg, 2); >> if (ret == 2) { >> memcpy(val, &buf[reg], len); >> > > -- http://palosaari.fi/