[PATCH] drm/i2c: tda998x: Remove VLA usage

[PATCH] drm/i2c: tda998x: Remove VLA usage

[Laura Abbott]

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>
---
This one really feels like it should be a #define but I wasn't sure
where the 32 came from. It looks like most other uses use one of the
#defines in include/linux/hdmi?
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9e67a7b4e3a4..29e2f49601c7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -470,7 +470,8 @@ 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[33];
 	int ret;
 
 	buf[0] = REG2ADDR(reg);
-- 
2.14.3

Re: [PATCH] drm/i2c: tda998x: Remove VLA usage

[Russell King - ARM Linux]

On Mon, Apr 09, 2018 at 02:07:03PM -0700, Laura Abbott 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.

Does this _really_ make it safer?  What if the code is modified to write
more than 32 bytes in the future?

Sorry, I don't think this is safer at all.

-- 
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

Re: [PATCH] drm/i2c: tda998x: Remove VLA usage

[Laura Abbott]

On 04/09/2018 03:21 PM, Russell King - ARM Linux wrote:
> On Mon, Apr 09, 2018 at 02:07:03PM -0700, Laura Abbott 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.
> 
> Does this _really_ make it safer?  What if the code is modified to write
> more than 32 bytes in the future?
> 
> Sorry, I don't think this is safer at all.
> 

Yeah I wasn't 100% sure about this one. Elsewhere, we've added bounds
checks against the new static size buffer so we could do that here
to ensure we don't overrun the stack if we do need to write more
than 32 bytes in the future. Another option is to switch to
a kmalloc buffer. Are either of those options acceptable to you or
do you have a better idea of how to get rid of the VLA?

Thanks,
Laura

Re: [PATCH] drm/i2c: tda998x: Remove VLA usage

[Russell King - ARM Linux]

On Tue, Apr 10, 2018 at 02:52:35PM -0700, Laura Abbott wrote:
> On 04/09/2018 03:21 PM, Russell King - ARM Linux wrote:
> >On Mon, Apr 09, 2018 at 02:07:03PM -0700, Laura Abbott 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.
> >
> >Does this _really_ make it safer?  What if the code is modified to write
> >more than 32 bytes in the future?
> >
> >Sorry, I don't think this is safer at all.
> >
> 
> Yeah I wasn't 100% sure about this one. Elsewhere, we've added bounds
> checks against the new static size buffer so we could do that here
> to ensure we don't overrun the stack if we do need to write more
> than 32 bytes in the future. Another option is to switch to
> a kmalloc buffer. Are either of those options acceptable to you or
> do you have a better idea of how to get rid of the VLA?

Limiting the size would be better (with an error message/WARN_ON) -
at least that results in a diagnostic message to alert the developer
rather than silently stomping over the stack.

-- 
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