| Message ID | 1383399097-11615-20-git-send-email-m.chehab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Acked-by: Antti Palosaari <crope@iki.fi> Reviewed-by: Antti Palosaari <crope@iki.fi> Antti On 02.11.2013 15:31, 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 80, it seem safe to use 80 as the hard limit for all > those devices. On most cases, the limit is a way lower than that, but > 80 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 <m.chehab@samsung.com> > Cc: Antti Palosaari <crope@iki.fi> > --- > drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > 4 files changed, 64 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > index ad9309da4a91..235e90251609 100644 > --- a/drivers/media/tuners/e4000.c > +++ b/drivers/media/tuners/e4000.c > @@ -24,7 +24,7 @@ > static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -34,6 +34,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 -EREMOTEIO; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -53,7 +60,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[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -68,6 +75,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 -EREMOTEIO; > + } > + > 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..e27bf5be311d 100644 > --- a/drivers/media/tuners/fc2580.c > +++ b/drivers/media/tuners/fc2580.c > @@ -41,7 +41,7 @@ > static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -51,6 +51,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 -EREMOTEIO; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -69,7 +76,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[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -84,6 +91,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 -EREMOTEIO; > + } > + > 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..765b9f9d4bc6 100644 > --- a/drivers/media/tuners/tda18212.c > +++ b/drivers/media/tuners/tda18212.c > @@ -32,7 +32,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len+1]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -42,6 +42,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 -EREMOTEIO; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -61,7 +68,7 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len]; > + u8 buf[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -76,6 +83,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 -EREMOTEIO; > + } > + > 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..e4e662c2e6ef 100644 > --- a/drivers/media/tuners/tda18218.c > +++ b/drivers/media/tuners/tda18218.c > @@ -24,7 +24,7 @@ > 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[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -33,6 +33,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 -EREMOTEIO; > + } > + > for (remaining = len; remaining > 0; > remaining -= (priv->cfg->i2c_wr_max - 1)) { > len2 = remaining; > @@ -63,7 +70,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[80]; /* we must start read always from reg 0x00 */ > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -78,6 +85,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 -EREMOTEIO; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, &buf[reg], len); >
Hi Mauro, I'll review this series more carefully on Monday, but for now I want to make a suggestion for the array checks: On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all > those devices. On most cases, the limit is a way lower than that, but > 80 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 <m.chehab@samsung.com> > Cc: Antti Palosaari <crope@iki.fi> > --- > drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > 4 files changed, 64 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > index ad9309da4a91..235e90251609 100644 > --- a/drivers/media/tuners/e4000.c > +++ b/drivers/media/tuners/e4000.c > @@ -24,7 +24,7 @@ > static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -34,6 +34,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 -EREMOTEIO; > + } > + I think this can be greatly simplified to: if (WARN_ON(len + 1 > sizeof(buf)) return -EREMOTEIO; This should really never happen, and it is a clear driver bug if it does. WARN_ON does the job IMHO. I also don't like the EREMOTEIO error: it has nothing to do with an I/O problem. Wouldn't EMSGSIZE be much better here? Ditto for all the similar situations in the patch series. Regards, Hans > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -53,7 +60,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[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -68,6 +75,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 -EREMOTEIO; > + } > + > 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..e27bf5be311d 100644 > --- a/drivers/media/tuners/fc2580.c > +++ b/drivers/media/tuners/fc2580.c > @@ -41,7 +41,7 @@ > static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > { > int ret; > - u8 buf[1 + len]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_addr, > @@ -51,6 +51,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 -EREMOTEIO; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -69,7 +76,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[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_addr, > @@ -84,6 +91,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 -EREMOTEIO; > + } > + > 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..765b9f9d4bc6 100644 > --- a/drivers/media/tuners/tda18212.c > +++ b/drivers/media/tuners/tda18212.c > @@ -32,7 +32,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len+1]; > + u8 buf[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -42,6 +42,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 -EREMOTEIO; > + } > + > buf[0] = reg; > memcpy(&buf[1], val, len); > > @@ -61,7 +68,7 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > int len) > { > int ret; > - u8 buf[len]; > + u8 buf[80]; > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -76,6 +83,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 -EREMOTEIO; > + } > + > 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..e4e662c2e6ef 100644 > --- a/drivers/media/tuners/tda18218.c > +++ b/drivers/media/tuners/tda18218.c > @@ -24,7 +24,7 @@ > 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[80]; > struct i2c_msg msg[1] = { > { > .addr = priv->cfg->i2c_address, > @@ -33,6 +33,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 -EREMOTEIO; > + } > + > for (remaining = len; remaining > 0; > remaining -= (priv->cfg->i2c_wr_max - 1)) { > len2 = remaining; > @@ -63,7 +70,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[80]; /* we must start read always from reg 0x00 */ > struct i2c_msg msg[2] = { > { > .addr = priv->cfg->i2c_address, > @@ -78,6 +85,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 -EREMOTEIO; > + } > + > ret = i2c_transfer(priv->i2c, msg, 2); > if (ret == 2) { > memcpy(val, &buf[reg], len); > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 02 Nov 2013 18:25:19 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > Hi Mauro, > > I'll review this series more carefully on Monday, Thanks! > but for now I want to make > a suggestion for the array checks: > > On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all > > those devices. On most cases, the limit is a way lower than that, but > > 80 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 <m.chehab@samsung.com> > > Cc: Antti Palosaari <crope@iki.fi> > > --- > > drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > > drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > > drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > > drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > > 4 files changed, 64 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > > index ad9309da4a91..235e90251609 100644 > > --- a/drivers/media/tuners/e4000.c > > +++ b/drivers/media/tuners/e4000.c > > @@ -24,7 +24,7 @@ > > static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > > { > > int ret; > > - u8 buf[1 + len]; > > + u8 buf[80]; > > struct i2c_msg msg[1] = { > > { > > .addr = priv->cfg->i2c_addr, > > @@ -34,6 +34,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 -EREMOTEIO; > > + } > > + > > I think this can be greatly simplified to: > > if (WARN_ON(len + 1 > sizeof(buf)) > return -EREMOTEIO; > > This should really never happen, and it is a clear driver bug if it does. WARN_ON > does the job IMHO. Works for me. I'll wait for more comments, and go for it on v3. > I also don't like the EREMOTEIO error: it has nothing to do with > an I/O problem. Wouldn't EMSGSIZE be much better here? EMSGSIZE is not used yet at drivers/media. So, it is probably not the right error code. I remember that there's an error code for that on I2C (EOPNOTSUPP?). In any case, I don't think we should use an unusual error code here. In theory, this error should never happen, but we don't want to break userspace because of it. That's why I opted to use EREMOTEIO: this is the error code that most of those drivers return when something gets wrong during I2C transfers. > Ditto for all the similar situations in the patch series. > > Regards, > > Hans > > > buf[0] = reg; > > memcpy(&buf[1], val, len); > > > > @@ -53,7 +60,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[80]; > > struct i2c_msg msg[2] = { > > { > > .addr = priv->cfg->i2c_addr, > > @@ -68,6 +75,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 -EREMOTEIO; > > + } > > + > > 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..e27bf5be311d 100644 > > --- a/drivers/media/tuners/fc2580.c > > +++ b/drivers/media/tuners/fc2580.c > > @@ -41,7 +41,7 @@ > > static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) > > { > > int ret; > > - u8 buf[1 + len]; > > + u8 buf[80]; > > struct i2c_msg msg[1] = { > > { > > .addr = priv->cfg->i2c_addr, > > @@ -51,6 +51,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 -EREMOTEIO; > > + } > > + > > buf[0] = reg; > > memcpy(&buf[1], val, len); > > > > @@ -69,7 +76,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[80]; > > struct i2c_msg msg[2] = { > > { > > .addr = priv->cfg->i2c_addr, > > @@ -84,6 +91,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 -EREMOTEIO; > > + } > > + > > 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..765b9f9d4bc6 100644 > > --- a/drivers/media/tuners/tda18212.c > > +++ b/drivers/media/tuners/tda18212.c > > @@ -32,7 +32,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > > int len) > > { > > int ret; > > - u8 buf[len+1]; > > + u8 buf[80]; > > struct i2c_msg msg[1] = { > > { > > .addr = priv->cfg->i2c_address, > > @@ -42,6 +42,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 -EREMOTEIO; > > + } > > + > > buf[0] = reg; > > memcpy(&buf[1], val, len); > > > > @@ -61,7 +68,7 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, > > int len) > > { > > int ret; > > - u8 buf[len]; > > + u8 buf[80]; > > struct i2c_msg msg[2] = { > > { > > .addr = priv->cfg->i2c_address, > > @@ -76,6 +83,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 -EREMOTEIO; > > + } > > + > > 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..e4e662c2e6ef 100644 > > --- a/drivers/media/tuners/tda18218.c > > +++ b/drivers/media/tuners/tda18218.c > > @@ -24,7 +24,7 @@ > > 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[80]; > > struct i2c_msg msg[1] = { > > { > > .addr = priv->cfg->i2c_address, > > @@ -33,6 +33,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 -EREMOTEIO; > > + } > > + > > for (remaining = len; remaining > 0; > > remaining -= (priv->cfg->i2c_wr_max - 1)) { > > len2 = remaining; > > @@ -63,7 +70,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[80]; /* we must start read always from reg 0x00 */ > > struct i2c_msg msg[2] = { > > { > > .addr = priv->cfg->i2c_address, > > @@ -78,6 +85,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 -EREMOTEIO; > > + } > > + > > ret = i2c_transfer(priv->i2c, msg, 2); > > if (ret == 2) { > > memcpy(val, &buf[reg], len); > > >
On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: > Em Sat, 02 Nov 2013 18:25:19 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> Hi Mauro, >> >> I'll review this series more carefully on Monday, > > Thanks! > >> but for now I want to make >> a suggestion for the array checks: >> >> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all >>> those devices. On most cases, the limit is a way lower than that, but >>> 80 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 <m.chehab@samsung.com> >>> Cc: Antti Palosaari <crope@iki.fi> >>> --- >>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- >>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- >>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- >>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- >>> 4 files changed, 64 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >>> index ad9309da4a91..235e90251609 100644 >>> --- a/drivers/media/tuners/e4000.c >>> +++ b/drivers/media/tuners/e4000.c >>> @@ -24,7 +24,7 @@ >>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) >>> { >>> int ret; >>> - u8 buf[1 + len]; >>> + u8 buf[80]; >>> struct i2c_msg msg[1] = { >>> { >>> .addr = priv->cfg->i2c_addr, >>> @@ -34,6 +34,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 -EREMOTEIO; >>> + } >>> + >> >> I think this can be greatly simplified to: >> >> if (WARN_ON(len + 1 > sizeof(buf)) >> return -EREMOTEIO; >> >> This should really never happen, and it is a clear driver bug if it does. WARN_ON >> does the job IMHO. > > Works for me. I'll wait for more comments, and go for it on v3. > >> I also don't like the EREMOTEIO error: it has nothing to do with >> an I/O problem. Wouldn't EMSGSIZE be much better here? > > > EMSGSIZE is not used yet at drivers/media. So, it is probably not the > right error code. > > I remember that there's an error code for that on I2C (EOPNOTSUPP?). > > In any case, I don't think we should use an unusual error code here. > In theory, this error should never happen, but we don't want to break > userspace because of it. That's why I opted to use EREMOTEIO: this is > the error code that most of those drivers return when something gets > wrong during I2C transfers. The problem I have is that EREMOTEIO is used when the i2c transfer fails, i.e. there is some sort of a hardware or communication problem. That's not the case here, it's an argument error. So EINVAL would actually be better, but that's perhaps overused which is why I suggested EMSGSIZE. I personally don't think EIO or EREMOTEIO should be used for something that is not hardware related. I'm sure there are some gray areas, but this particular situation is clearly not hardware-related. So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. ENOMEM is also an option (you are after all 'out of buffer memory'). A bit more exotic, but still sort of in the area, is EPROTO. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/02/2013 10:53 PM, Hans Verkuil wrote: > On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: >> Em Sat, 02 Nov 2013 18:25:19 +0100 >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >> >>> Hi Mauro, >>> >>> I'll review this series more carefully on Monday, >> >> Thanks! >> >>> but for now I want to make >>> a suggestion for the array checks: >>> >>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all >>>> those devices. On most cases, the limit is a way lower than that, but >>>> 80 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 <m.chehab@samsung.com> >>>> Cc: Antti Palosaari <crope@iki.fi> >>>> --- >>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- >>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- >>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- >>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- >>>> 4 files changed, 64 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >>>> index ad9309da4a91..235e90251609 100644 >>>> --- a/drivers/media/tuners/e4000.c >>>> +++ b/drivers/media/tuners/e4000.c >>>> @@ -24,7 +24,7 @@ >>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) >>>> { >>>> int ret; >>>> - u8 buf[1 + len]; >>>> + u8 buf[80]; >>>> struct i2c_msg msg[1] = { >>>> { >>>> .addr = priv->cfg->i2c_addr, >>>> @@ -34,6 +34,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 -EREMOTEIO; >>>> + } >>>> + >>> >>> I think this can be greatly simplified to: >>> >>> if (WARN_ON(len + 1 > sizeof(buf)) >>> return -EREMOTEIO; >>> >>> This should really never happen, and it is a clear driver bug if it does. WARN_ON >>> does the job IMHO. >> >> Works for me. I'll wait for more comments, and go for it on v3. >> >>> I also don't like the EREMOTEIO error: it has nothing to do with >>> an I/O problem. Wouldn't EMSGSIZE be much better here? >> >> >> EMSGSIZE is not used yet at drivers/media. So, it is probably not the >> right error code. >> >> I remember that there's an error code for that on I2C (EOPNOTSUPP?). >> >> In any case, I don't think we should use an unusual error code here. >> In theory, this error should never happen, but we don't want to break >> userspace because of it. That's why I opted to use EREMOTEIO: this is >> the error code that most of those drivers return when something gets >> wrong during I2C transfers. > > The problem I have is that EREMOTEIO is used when the i2c transfer fails, > i.e. there is some sort of a hardware or communication problem. > > That's not the case here, it's an argument error. So EINVAL would actually > be better, but that's perhaps overused which is why I suggested EMSGSIZE. > I personally don't think EIO or EREMOTEIO should be used for something that > is not hardware related. I'm sure there are some gray areas, but this > particular situation is clearly not hardware-related. > > So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. > ENOMEM is also an option (you are after all 'out of buffer memory'). > A bit more exotic, but still sort of in the area, is EPROTO. After thinking about it a little bit more I would just return -EINVAL. It's a wrong argument, it's something that shouldn't happen at all, and you get a big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 02 Nov 2013 22:59:04 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 11/02/2013 10:53 PM, Hans Verkuil wrote: > > On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: > >> Em Sat, 02 Nov 2013 18:25:19 +0100 > >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> > >>> Hi Mauro, > >>> > >>> I'll review this series more carefully on Monday, > >> > >> Thanks! > >> > >>> but for now I want to make > >>> a suggestion for the array checks: > >>> > >>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all > >>>> those devices. On most cases, the limit is a way lower than that, but > >>>> 80 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 <m.chehab@samsung.com> > >>>> Cc: Antti Palosaari <crope@iki.fi> > >>>> --- > >>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > >>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > >>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > >>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > >>>> 4 files changed, 64 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > >>>> index ad9309da4a91..235e90251609 100644 > >>>> --- a/drivers/media/tuners/e4000.c > >>>> +++ b/drivers/media/tuners/e4000.c > >>>> @@ -24,7 +24,7 @@ > >>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > >>>> { > >>>> int ret; > >>>> - u8 buf[1 + len]; > >>>> + u8 buf[80]; > >>>> struct i2c_msg msg[1] = { > >>>> { > >>>> .addr = priv->cfg->i2c_addr, > >>>> @@ -34,6 +34,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 -EREMOTEIO; > >>>> + } > >>>> + > >>> > >>> I think this can be greatly simplified to: > >>> > >>> if (WARN_ON(len + 1 > sizeof(buf)) > >>> return -EREMOTEIO; > >>> > >>> This should really never happen, and it is a clear driver bug if it does. WARN_ON > >>> does the job IMHO. > >> > >> Works for me. I'll wait for more comments, and go for it on v3. > >> > >>> I also don't like the EREMOTEIO error: it has nothing to do with > >>> an I/O problem. Wouldn't EMSGSIZE be much better here? > >> > >> > >> EMSGSIZE is not used yet at drivers/media. So, it is probably not the > >> right error code. > >> > >> I remember that there's an error code for that on I2C (EOPNOTSUPP?). > >> > >> In any case, I don't think we should use an unusual error code here. > >> In theory, this error should never happen, but we don't want to break > >> userspace because of it. That's why I opted to use EREMOTEIO: this is > >> the error code that most of those drivers return when something gets > >> wrong during I2C transfers. > > > > The problem I have is that EREMOTEIO is used when the i2c transfer fails, > > i.e. there is some sort of a hardware or communication problem. > > > > That's not the case here, it's an argument error. So EINVAL would actually > > be better, but that's perhaps overused which is why I suggested EMSGSIZE. > > I personally don't think EIO or EREMOTEIO should be used for something that > > is not hardware related. I'm sure there are some gray areas, but this > > particular situation is clearly not hardware-related. > > > > So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. > > ENOMEM is also an option (you are after all 'out of buffer memory'). > > A bit more exotic, but still sort of in the area, is EPROTO. > > After thinking about it a little bit more I would just return -EINVAL. It's > a wrong argument, it's something that shouldn't happen at all, and you get a > big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. Works for me. Regards, Mauro > > Regards, > > Hans
Em Sat, 2 Nov 2013 22:21:32 -0200 Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu: > Em Sat, 02 Nov 2013 22:59:04 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > On 11/02/2013 10:53 PM, Hans Verkuil wrote: > > > On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: > > >> Em Sat, 02 Nov 2013 18:25:19 +0100 > > >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > >> > > >>> Hi Mauro, > > >>> > > >>> I'll review this series more carefully on Monday, > > >> > > >> Thanks! > > >> > > >>> but for now I want to make > > >>> a suggestion for the array checks: > > >>> > > >>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all > > >>>> those devices. On most cases, the limit is a way lower than that, but > > >>>> 80 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 <m.chehab@samsung.com> > > >>>> Cc: Antti Palosaari <crope@iki.fi> > > >>>> --- > > >>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > > >>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > > >>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > > >>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > > >>>> 4 files changed, 64 insertions(+), 8 deletions(-) > > >>>> > > >>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > > >>>> index ad9309da4a91..235e90251609 100644 > > >>>> --- a/drivers/media/tuners/e4000.c > > >>>> +++ b/drivers/media/tuners/e4000.c > > >>>> @@ -24,7 +24,7 @@ > > >>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > > >>>> { > > >>>> int ret; > > >>>> - u8 buf[1 + len]; > > >>>> + u8 buf[80]; > > >>>> struct i2c_msg msg[1] = { > > >>>> { > > >>>> .addr = priv->cfg->i2c_addr, > > >>>> @@ -34,6 +34,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 -EREMOTEIO; > > >>>> + } > > >>>> + > > >>> > > >>> I think this can be greatly simplified to: > > >>> > > >>> if (WARN_ON(len + 1 > sizeof(buf)) > > >>> return -EREMOTEIO; > > >>> > > >>> This should really never happen, and it is a clear driver bug if it does. WARN_ON > > >>> does the job IMHO. > > >> > > >> Works for me. I'll wait for more comments, and go for it on v3. > > >> > > >>> I also don't like the EREMOTEIO error: it has nothing to do with > > >>> an I/O problem. Wouldn't EMSGSIZE be much better here? > > >> > > >> > > >> EMSGSIZE is not used yet at drivers/media. So, it is probably not the > > >> right error code. > > >> > > >> I remember that there's an error code for that on I2C (EOPNOTSUPP?). > > >> > > >> In any case, I don't think we should use an unusual error code here. > > >> In theory, this error should never happen, but we don't want to break > > >> userspace because of it. That's why I opted to use EREMOTEIO: this is > > >> the error code that most of those drivers return when something gets > > >> wrong during I2C transfers. > > > > > > The problem I have is that EREMOTEIO is used when the i2c transfer fails, > > > i.e. there is some sort of a hardware or communication problem. > > > > > > That's not the case here, it's an argument error. So EINVAL would actually > > > be better, but that's perhaps overused which is why I suggested EMSGSIZE. > > > I personally don't think EIO or EREMOTEIO should be used for something that > > > is not hardware related. I'm sure there are some gray areas, but this > > > particular situation is clearly not hardware-related. > > > > > > So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. > > > ENOMEM is also an option (you are after all 'out of buffer memory'). > > > A bit more exotic, but still sort of in the area, is EPROTO. > > > > After thinking about it a little bit more I would just return -EINVAL. It's > > a wrong argument, it's something that shouldn't happen at all, and you get a > > big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. > > Works for me. After thinking a little bit about that, I think that using WARN_ON is not a good idea. The thing is that userspace may access directly the I2C devices, via i2c-dev, and try to read/write using more data than supported. On such cases, the expected behavior is for the driver to return EOPNOTSUPP without generating a WARN_ON dump. So, IMHO, the better is to keep the patches as-is, and just replace the return code to EOPNOTSUPP, if the size is bigger than supported. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03.11.2013 11:12, Mauro Carvalho Chehab wrote: > Em Sat, 2 Nov 2013 22:21:32 -0200 > Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu: > >> Em Sat, 02 Nov 2013 22:59:04 +0100 >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >> >>> On 11/02/2013 10:53 PM, Hans Verkuil wrote: >>>> On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: >>>>> Em Sat, 02 Nov 2013 18:25:19 +0100 >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >>>>> >>>>>> Hi Mauro, >>>>>> >>>>>> I'll review this series more carefully on Monday, >>>>> >>>>> Thanks! >>>>> >>>>>> but for now I want to make >>>>>> a suggestion for the array checks: >>>>>> >>>>>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all >>>>>>> those devices. On most cases, the limit is a way lower than that, but >>>>>>> 80 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 <m.chehab@samsung.com> >>>>>>> Cc: Antti Palosaari <crope@iki.fi> >>>>>>> --- >>>>>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- >>>>>>> 4 files changed, 64 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >>>>>>> index ad9309da4a91..235e90251609 100644 >>>>>>> --- a/drivers/media/tuners/e4000.c >>>>>>> +++ b/drivers/media/tuners/e4000.c >>>>>>> @@ -24,7 +24,7 @@ >>>>>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) >>>>>>> { >>>>>>> int ret; >>>>>>> - u8 buf[1 + len]; >>>>>>> + u8 buf[80]; >>>>>>> struct i2c_msg msg[1] = { >>>>>>> { >>>>>>> .addr = priv->cfg->i2c_addr, >>>>>>> @@ -34,6 +34,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 -EREMOTEIO; >>>>>>> + } >>>>>>> + >>>>>> >>>>>> I think this can be greatly simplified to: >>>>>> >>>>>> if (WARN_ON(len + 1 > sizeof(buf)) >>>>>> return -EREMOTEIO; >>>>>> >>>>>> This should really never happen, and it is a clear driver bug if it does. WARN_ON >>>>>> does the job IMHO. >>>>> >>>>> Works for me. I'll wait for more comments, and go for it on v3. >>>>> >>>>>> I also don't like the EREMOTEIO error: it has nothing to do with >>>>>> an I/O problem. Wouldn't EMSGSIZE be much better here? >>>>> >>>>> >>>>> EMSGSIZE is not used yet at drivers/media. So, it is probably not the >>>>> right error code. >>>>> >>>>> I remember that there's an error code for that on I2C (EOPNOTSUPP?). >>>>> >>>>> In any case, I don't think we should use an unusual error code here. >>>>> In theory, this error should never happen, but we don't want to break >>>>> userspace because of it. That's why I opted to use EREMOTEIO: this is >>>>> the error code that most of those drivers return when something gets >>>>> wrong during I2C transfers. >>>> >>>> The problem I have is that EREMOTEIO is used when the i2c transfer fails, >>>> i.e. there is some sort of a hardware or communication problem. >>>> >>>> That's not the case here, it's an argument error. So EINVAL would actually >>>> be better, but that's perhaps overused which is why I suggested EMSGSIZE. >>>> I personally don't think EIO or EREMOTEIO should be used for something that >>>> is not hardware related. I'm sure there are some gray areas, but this >>>> particular situation is clearly not hardware-related. >>>> >>>> So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. >>>> ENOMEM is also an option (you are after all 'out of buffer memory'). >>>> A bit more exotic, but still sort of in the area, is EPROTO. >>> >>> After thinking about it a little bit more I would just return -EINVAL. It's >>> a wrong argument, it's something that shouldn't happen at all, and you get a >>> big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. >> >> Works for me. > > After thinking a little bit about that, I think that using WARN_ON is not > a good idea. > > The thing is that userspace may access directly the I2C devices, via > i2c-dev, and try to read/write using more data than supported. On such cases, > the expected behavior is for the driver to return EOPNOTSUPP without generating > a WARN_ON dump. > > So, IMHO, the better is to keep the patches as-is, and just replace the > return code to EOPNOTSUPP, if the size is bigger than supported. There should be checks in every I2C adapter, which returns -EOPNOTSUPP, when unsupported message is coming. It is another issue. For adapters EOPNOTSUPP is suitable error code but for client in that case it is not. If that happens in client it is simply driver bug and I think EINVAL in suitable. regards Antti
On 11/03/2013 10:12 AM, Mauro Carvalho Chehab wrote: > Em Sat, 2 Nov 2013 22:21:32 -0200 > Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu: > >> Em Sat, 02 Nov 2013 22:59:04 +0100 >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >> >>> On 11/02/2013 10:53 PM, Hans Verkuil wrote: >>>> On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: >>>>> Em Sat, 02 Nov 2013 18:25:19 +0100 >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >>>>> >>>>>> Hi Mauro, >>>>>> >>>>>> I'll review this series more carefully on Monday, >>>>> >>>>> Thanks! >>>>> >>>>>> but for now I want to make >>>>>> a suggestion for the array checks: >>>>>> >>>>>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all >>>>>>> those devices. On most cases, the limit is a way lower than that, but >>>>>>> 80 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 <m.chehab@samsung.com> >>>>>>> Cc: Antti Palosaari <crope@iki.fi> >>>>>>> --- >>>>>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- >>>>>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- >>>>>>> 4 files changed, 64 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c >>>>>>> index ad9309da4a91..235e90251609 100644 >>>>>>> --- a/drivers/media/tuners/e4000.c >>>>>>> +++ b/drivers/media/tuners/e4000.c >>>>>>> @@ -24,7 +24,7 @@ >>>>>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) >>>>>>> { >>>>>>> int ret; >>>>>>> - u8 buf[1 + len]; >>>>>>> + u8 buf[80]; >>>>>>> struct i2c_msg msg[1] = { >>>>>>> { >>>>>>> .addr = priv->cfg->i2c_addr, >>>>>>> @@ -34,6 +34,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 -EREMOTEIO; >>>>>>> + } >>>>>>> + >>>>>> >>>>>> I think this can be greatly simplified to: >>>>>> >>>>>> if (WARN_ON(len + 1 > sizeof(buf)) >>>>>> return -EREMOTEIO; >>>>>> >>>>>> This should really never happen, and it is a clear driver bug if it does. WARN_ON >>>>>> does the job IMHO. >>>>> >>>>> Works for me. I'll wait for more comments, and go for it on v3. >>>>> >>>>>> I also don't like the EREMOTEIO error: it has nothing to do with >>>>>> an I/O problem. Wouldn't EMSGSIZE be much better here? >>>>> >>>>> >>>>> EMSGSIZE is not used yet at drivers/media. So, it is probably not the >>>>> right error code. >>>>> >>>>> I remember that there's an error code for that on I2C (EOPNOTSUPP?). >>>>> >>>>> In any case, I don't think we should use an unusual error code here. >>>>> In theory, this error should never happen, but we don't want to break >>>>> userspace because of it. That's why I opted to use EREMOTEIO: this is >>>>> the error code that most of those drivers return when something gets >>>>> wrong during I2C transfers. >>>> >>>> The problem I have is that EREMOTEIO is used when the i2c transfer fails, >>>> i.e. there is some sort of a hardware or communication problem. >>>> >>>> That's not the case here, it's an argument error. So EINVAL would actually >>>> be better, but that's perhaps overused which is why I suggested EMSGSIZE. >>>> I personally don't think EIO or EREMOTEIO should be used for something that >>>> is not hardware related. I'm sure there are some gray areas, but this >>>> particular situation is clearly not hardware-related. >>>> >>>> So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. >>>> ENOMEM is also an option (you are after all 'out of buffer memory'). >>>> A bit more exotic, but still sort of in the area, is EPROTO. >>> >>> After thinking about it a little bit more I would just return -EINVAL. It's >>> a wrong argument, it's something that shouldn't happen at all, and you get a >>> big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. >> >> Works for me. > > After thinking a little bit about that, I think that using WARN_ON is not > a good idea. > > The thing is that userspace may access directly the I2C devices, via > i2c-dev, and try to read/write using more data than supported. On such cases, > the expected behavior is for the driver to return EOPNOTSUPP without generating > a WARN_ON dump. Fair enough. I hadn't thought of that. > So, IMHO, the better is to keep the patches as-is, and just replace the > return code to EOPNOTSUPP, if the size is bigger than supported. I still think that EINVAL is the right error code here. I also wonder whether you really should print anything if this happens. Looking at the patch series it adds many such warnings in lots of drivers. I see this as an exceedingly rare thing, and just returning an error should be sufficient. In the case of i2c-dev I don't think you want to print anything anyway if the user provides too much data: it's generally not a good idea to print warnings in the kernel log in case of incorrect user input. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, 04 Nov 2013 14:26:33 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 11/03/2013 10:12 AM, Mauro Carvalho Chehab wrote: > > Em Sat, 2 Nov 2013 22:21:32 -0200 > > Mauro Carvalho Chehab <m.chehab@samsung.com> escreveu: > > > >> Em Sat, 02 Nov 2013 22:59:04 +0100 > >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> > >>> On 11/02/2013 10:53 PM, Hans Verkuil wrote: > >>>> On 11/02/2013 10:15 PM, Mauro Carvalho Chehab wrote: > >>>>> Em Sat, 02 Nov 2013 18:25:19 +0100 > >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >>>>> > >>>>>> Hi Mauro, > >>>>>> > >>>>>> I'll review this series more carefully on Monday, > >>>>> > >>>>> Thanks! > >>>>> > >>>>>> but for now I want to make > >>>>>> a suggestion for the array checks: > >>>>>> > >>>>>> On 11/02/2013 02:31 PM, 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 80, it seem safe to use 80 as the hard limit for all > >>>>>>> those devices. On most cases, the limit is a way lower than that, but > >>>>>>> 80 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 <m.chehab@samsung.com> > >>>>>>> Cc: Antti Palosaari <crope@iki.fi> > >>>>>>> --- > >>>>>>> drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- > >>>>>>> drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- > >>>>>>> drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- > >>>>>>> drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- > >>>>>>> 4 files changed, 64 insertions(+), 8 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c > >>>>>>> index ad9309da4a91..235e90251609 100644 > >>>>>>> --- a/drivers/media/tuners/e4000.c > >>>>>>> +++ b/drivers/media/tuners/e4000.c > >>>>>>> @@ -24,7 +24,7 @@ > >>>>>>> static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) > >>>>>>> { > >>>>>>> int ret; > >>>>>>> - u8 buf[1 + len]; > >>>>>>> + u8 buf[80]; > >>>>>>> struct i2c_msg msg[1] = { > >>>>>>> { > >>>>>>> .addr = priv->cfg->i2c_addr, > >>>>>>> @@ -34,6 +34,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 -EREMOTEIO; > >>>>>>> + } > >>>>>>> + > >>>>>> > >>>>>> I think this can be greatly simplified to: > >>>>>> > >>>>>> if (WARN_ON(len + 1 > sizeof(buf)) > >>>>>> return -EREMOTEIO; > >>>>>> > >>>>>> This should really never happen, and it is a clear driver bug if it does. WARN_ON > >>>>>> does the job IMHO. > >>>>> > >>>>> Works for me. I'll wait for more comments, and go for it on v3. > >>>>> > >>>>>> I also don't like the EREMOTEIO error: it has nothing to do with > >>>>>> an I/O problem. Wouldn't EMSGSIZE be much better here? > >>>>> > >>>>> > >>>>> EMSGSIZE is not used yet at drivers/media. So, it is probably not the > >>>>> right error code. > >>>>> > >>>>> I remember that there's an error code for that on I2C (EOPNOTSUPP?). > >>>>> > >>>>> In any case, I don't think we should use an unusual error code here. > >>>>> In theory, this error should never happen, but we don't want to break > >>>>> userspace because of it. That's why I opted to use EREMOTEIO: this is > >>>>> the error code that most of those drivers return when something gets > >>>>> wrong during I2C transfers. > >>>> > >>>> The problem I have is that EREMOTEIO is used when the i2c transfer fails, > >>>> i.e. there is some sort of a hardware or communication problem. > >>>> > >>>> That's not the case here, it's an argument error. So EINVAL would actually > >>>> be better, but that's perhaps overused which is why I suggested EMSGSIZE. > >>>> I personally don't think EIO or EREMOTEIO should be used for something that > >>>> is not hardware related. I'm sure there are some gray areas, but this > >>>> particular situation is clearly not hardware-related. > >>>> > >>>> So if EMSGSIZE won't work for you, then I prefer EINVAL over EREMOTEIO. > >>>> ENOMEM is also an option (you are after all 'out of buffer memory'). > >>>> A bit more exotic, but still sort of in the area, is EPROTO. > >>> > >>> After thinking about it a little bit more I would just return -EINVAL. It's > >>> a wrong argument, it's something that shouldn't happen at all, and you get a > >>> big fat stack trace anyway due to the WARN_ON, so EINVAL makes perfect sense. > >> > >> Works for me. > > > > After thinking a little bit about that, I think that using WARN_ON is not > > a good idea. > > > > The thing is that userspace may access directly the I2C devices, via > > i2c-dev, and try to read/write using more data than supported. On such cases, > > the expected behavior is for the driver to return EOPNOTSUPP without generating > > a WARN_ON dump. > > Fair enough. I hadn't thought of that. > > > So, IMHO, the better is to keep the patches as-is, and just replace the > > return code to EOPNOTSUPP, if the size is bigger than supported. > > I still think that EINVAL is the right error code here. Ok. So, I'll use EINVAL for i2c client drivers, and EOPNOTSUPP on bridge ones. > I also wonder whether you really should print anything if this happens. Looking at > the patch series it adds many such warnings in lots of drivers. I see this as an > exceedingly rare thing, and just returning an error should be sufficient. Well, eventually, we might be putting a lower limit than needed. As this printk is rare enough, and it will help a lot on debugging issues at the driver if it ever happens, I think that the better is to add a printk here. > In the case of i2c-dev I don't think you want to print anything anyway if the user > provides too much data: it's generally not a good idea to print warnings in the kernel > log in case of incorrect user input. Agreed, but we can't distinguish if it happened due to i2c-dev or due to a bridge call. So, I think that having a single printk line (as opposite to a WARN_ON dump) to be a good compromise. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c index ad9309da4a91..235e90251609 100644 --- a/drivers/media/tuners/e4000.c +++ b/drivers/media/tuners/e4000.c @@ -24,7 +24,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len) { int ret; - u8 buf[1 + len]; + u8 buf[80]; struct i2c_msg msg[1] = { { .addr = priv->cfg->i2c_addr, @@ -34,6 +34,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 -EREMOTEIO; + } + buf[0] = reg; memcpy(&buf[1], val, len); @@ -53,7 +60,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[80]; struct i2c_msg msg[2] = { { .addr = priv->cfg->i2c_addr, @@ -68,6 +75,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 -EREMOTEIO; + } + 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..e27bf5be311d 100644 --- a/drivers/media/tuners/fc2580.c +++ b/drivers/media/tuners/fc2580.c @@ -41,7 +41,7 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len) { int ret; - u8 buf[1 + len]; + u8 buf[80]; struct i2c_msg msg[1] = { { .addr = priv->cfg->i2c_addr, @@ -51,6 +51,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 -EREMOTEIO; + } + buf[0] = reg; memcpy(&buf[1], val, len); @@ -69,7 +76,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[80]; struct i2c_msg msg[2] = { { .addr = priv->cfg->i2c_addr, @@ -84,6 +91,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 -EREMOTEIO; + } + 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..765b9f9d4bc6 100644 --- a/drivers/media/tuners/tda18212.c +++ b/drivers/media/tuners/tda18212.c @@ -32,7 +32,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val, int len) { int ret; - u8 buf[len+1]; + u8 buf[80]; struct i2c_msg msg[1] = { { .addr = priv->cfg->i2c_address, @@ -42,6 +42,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 -EREMOTEIO; + } + buf[0] = reg; memcpy(&buf[1], val, len); @@ -61,7 +68,7 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val, int len) { int ret; - u8 buf[len]; + u8 buf[80]; struct i2c_msg msg[2] = { { .addr = priv->cfg->i2c_address, @@ -76,6 +83,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 -EREMOTEIO; + } + 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..e4e662c2e6ef 100644 --- a/drivers/media/tuners/tda18218.c +++ b/drivers/media/tuners/tda18218.c @@ -24,7 +24,7 @@ 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[80]; struct i2c_msg msg[1] = { { .addr = priv->cfg->i2c_address, @@ -33,6 +33,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 -EREMOTEIO; + } + for (remaining = len; remaining > 0; remaining -= (priv->cfg->i2c_wr_max - 1)) { len2 = remaining; @@ -63,7 +70,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[80]; /* we must start read always from reg 0x00 */ struct i2c_msg msg[2] = { { .addr = priv->cfg->i2c_address, @@ -78,6 +85,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 -EREMOTEIO; + } + ret = i2c_transfer(priv->i2c, msg, 2); if (ret == 2) { memcpy(val, &buf[reg], len);
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 80, it seem safe to use 80 as the hard limit for all those devices. On most cases, the limit is a way lower than that, but 80 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 <m.chehab@samsung.com> Cc: Antti Palosaari <crope@iki.fi> --- drivers/media/tuners/e4000.c | 18 ++++++++++++++++-- drivers/media/tuners/fc2580.c | 18 ++++++++++++++++-- drivers/media/tuners/tda18212.c | 18 ++++++++++++++++-- drivers/media/tuners/tda18218.c | 18 ++++++++++++++++-- 4 files changed, 64 insertions(+), 8 deletions(-)