LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCHv2] drm/i2c: tda998x: Remove VLA usage
@ 2018-04-11  1:03 Laura Abbott
  2018-05-18 18:01 ` Kees Cook
  0 siblings, 1 reply; 4+ messages in thread
From: Laura Abbott @ 2018-04-11  1:03 UTC (permalink / raw)
  To: Russell King, David Airlie
  Cc: Laura Abbott, dri-devel, linux-kernel, kernel-hardening, Kees Cook

There's an ongoing effort to remove VLAs[1] from the kernel to eventually
turn on -Wvla. The vla in reg_write_range is based on the length of data
passed. The one use of a non-constant size for this range is bounded by
the size buffer passed to hdmi_infoframe_pack which is a fixed size.
Switch to this upper bound.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Switch to make the buffer size more transparent and add a bounds
check.
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9e67a7b4e3a4..c8b6029b7839 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -466,13 +466,22 @@ reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
 	return ret;
 }
 
+#define MAX_WRITE_RANGE_BUF 32
+
 static void
 reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
 {
 	struct i2c_client *client = priv->hdmi;
-	u8 buf[cnt+1];
+	/* This is the maximum size of the buffer passed in */
+	u8 buf[MAX_WRITE_RANGE_BUF + 1];
 	int ret;
 
+	if (cnt > MAX_WRITE_RANGE_BUF) {
+		dev_err(&client->dev, "Fixed write buffer too small (%d)\n",
+				MAX_WRITE_RANGE_BUF);
+		return;
+	}
+
 	buf[0] = REG2ADDR(reg);
 	memcpy(&buf[1], p, cnt);
 
@@ -679,7 +688,7 @@ static void
 tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 		 union hdmi_infoframe *frame)
 {
-	u8 buf[32];
+	u8 buf[MAX_WRITE_RANGE_BUF];
 	ssize_t len;
 
 	len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
-- 
2.14.3

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv2] drm/i2c: tda998x: Remove VLA usage
  2018-04-11  1:03 [PATCHv2] drm/i2c: tda998x: Remove VLA usage Laura Abbott
@ 2018-05-18 18:01 ` Kees Cook
  2018-05-19 10:07   ` Russell King - ARM Linux
  0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2018-05-18 18:01 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Laura Abbott, Russell King, David Airlie,
	Maling list - DRI developers, LKML, Kernel Hardening

On Tue, Apr 10, 2018 at 6:03 PM, Laura Abbott <labbott@redhat.com> wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. The vla in reg_write_range is based on the length of data
> passed. The one use of a non-constant size for this range is bounded by
> the size buffer passed to hdmi_infoframe_pack which is a fixed size.
> Switch to this upper bound.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

Same question for this patch: who's best to take this?

Thanks!

-Kees

> ---
> v2: Switch to make the buffer size more transparent and add a bounds
> check.
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 9e67a7b4e3a4..c8b6029b7839 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -466,13 +466,22 @@ reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
>         return ret;
>  }
>
> +#define MAX_WRITE_RANGE_BUF 32
> +
>  static void
>  reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
>  {
>         struct i2c_client *client = priv->hdmi;
> -       u8 buf[cnt+1];
> +       /* This is the maximum size of the buffer passed in */
> +       u8 buf[MAX_WRITE_RANGE_BUF + 1];
>         int ret;
>
> +       if (cnt > MAX_WRITE_RANGE_BUF) {
> +               dev_err(&client->dev, "Fixed write buffer too small (%d)\n",
> +                               MAX_WRITE_RANGE_BUF);
> +               return;
> +       }
> +
>         buf[0] = REG2ADDR(reg);
>         memcpy(&buf[1], p, cnt);
>
> @@ -679,7 +688,7 @@ static void
>  tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
>                  union hdmi_infoframe *frame)
>  {
> -       u8 buf[32];
> +       u8 buf[MAX_WRITE_RANGE_BUF];
>         ssize_t len;
>
>         len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
> --
> 2.14.3
>



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv2] drm/i2c: tda998x: Remove VLA usage
  2018-05-18 18:01 ` Kees Cook
@ 2018-05-19 10:07   ` Russell King - ARM Linux
  2018-05-23  8:46     ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Russell King - ARM Linux @ 2018-05-19 10:07 UTC (permalink / raw)
  To: Kees Cook
  Cc: Daniel Vetter, Laura Abbott, David Airlie,
	Maling list - DRI developers, LKML, Kernel Hardening

On Fri, May 18, 2018 at 11:01:55AM -0700, Kees Cook wrote:
> On Tue, Apr 10, 2018 at 6:03 PM, Laura Abbott <labbott@redhat.com> wrote:
> > There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> > turn on -Wvla. The vla in reg_write_range is based on the length of data
> > passed. The one use of a non-constant size for this range is bounded by
> > the size buffer passed to hdmi_infoframe_pack which is a fixed size.
> > Switch to this upper bound.
> >
> > [1] https://lkml.org/lkml/2018/3/7/621
> >
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> Same question for this patch: who's best to take this?

I had decided that I'm not taking any tda998x stuff until we get the
CEC support merged upstream, as that has been hanging around for ages.
Progress has been slow on that, but it finally got to the point where
everyone was happy with it, and I sent a pull request to David Airlie
on April 24th for it.

Unfortunately, that pull request has not been actioned to date.  I've
sent a chaser, and last night, I checked with David Airlie on IRC.
It seems David is not aware of my pull request.  David says he'll look
into this on Monday.

Until David does take it, I can't add anything further to my git tree
for tda998x development, as that would change what was sent to David
back in April.

The alternative would be for drm-misc to take it - I don't think it
will conflict with anything I've already asked David to take, so that
should be a safe route for _this_ patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCHv2] drm/i2c: tda998x: Remove VLA usage
  2018-05-19 10:07   ` Russell King - ARM Linux
@ 2018-05-23  8:46     ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2018-05-23  8:46 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Kees Cook, Daniel Vetter, Laura Abbott, David Airlie,
	Maling list - DRI developers, LKML, Kernel Hardening

On Sat, May 19, 2018 at 11:07:08AM +0100, Russell King - ARM Linux wrote:
> On Fri, May 18, 2018 at 11:01:55AM -0700, Kees Cook wrote:
> > On Tue, Apr 10, 2018 at 6:03 PM, Laura Abbott <labbott@redhat.com> wrote:
> > > There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> > > turn on -Wvla. The vla in reg_write_range is based on the length of data
> > > passed. The one use of a non-constant size for this range is bounded by
> > > the size buffer passed to hdmi_infoframe_pack which is a fixed size.
> > > Switch to this upper bound.
> > >
> > > [1] https://lkml.org/lkml/2018/3/7/621
> > >
> > > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> > Same question for this patch: who's best to take this?
> 
> I had decided that I'm not taking any tda998x stuff until we get the
> CEC support merged upstream, as that has been hanging around for ages.
> Progress has been slow on that, but it finally got to the point where
> everyone was happy with it, and I sent a pull request to David Airlie
> on April 24th for it.
> 
> Unfortunately, that pull request has not been actioned to date.  I've
> sent a chaser, and last night, I checked with David Airlie on IRC.
> It seems David is not aware of my pull request.  David says he'll look
> into this on Monday.
> 
> Until David does take it, I can't add anything further to my git tree
> for tda998x development, as that would change what was sent to David
> back in April.
> 
> The alternative would be for drm-misc to take it - I don't think it
> will conflict with anything I've already asked David to take, so that
> should be a safe route for _this_ patch.

Sounds reasonable, applied to drm-misc-next for 4.19 just to make sure it
won't get lost.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-23  8:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-11  1:03 [PATCHv2] drm/i2c: tda998x: Remove VLA usage Laura Abbott
2018-05-18 18:01 ` Kees Cook
2018-05-19 10:07   ` Russell King - ARM Linux
2018-05-23  8:46     ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).