usb: typec: tps6598x: Remove VLA usage

PATCH Superseded Kees Cook 2 participants 4 messages Jun 20, 2018 – Jun 25, 2018

Subsystems (2)

USB USB Type-C class

lore

Share & tools

lore

Kees Cook

author

2018-06-20 18:28 UTC #0 lore raw

PATCH 1/1 usb: typec: tps6598x: Remove VLA usage

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum buffer size and adds a sanity check.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Signed-off-by Kees Cook <[email protected]>

---
 drivers/usb/typec/tps6598x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/usb/typec/tps6598x.c +6 -1

index 4b4c8d271b27..396193f85e6d 100644
@@ -81,12 +81,17 @@ struct tps6598x {
 	struct typec_capability typec_cap;
 };
 
+#define TPS_MAX_LEN	sizeof(u64)
+
 static int
 tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 {
-	u8 data[len + 1];
+	u8 data[TPS_MAX_LEN + 1];
 	int ret;
 
+	if (WARN_ON(len + 1 > sizeof(data)))
+		return -EINVAL;
+
 	if (!tps->i2c_protocol)
 		return regmap_raw_read(tps->regmap, reg, val, len);

Heikki Krogerus

maintainer

2018-06-21 10:00 UTC #1 lore raw

Quoted 15 lines · Kees Cook

In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum buffer size and adds a sanity check.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/typec/tps6598x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/typec/tps6598x.c +0 -0
index 4b4c8d271b27..396193f85e6d 100644

drivers/usb/typec/tps6598x.c struct tps6598x { · line 84

Quoted 4 lines

drivers/usb/typec/tps6598x.c +1 -0

@@ -81,12 +81,17 @@ struct tps6598x {
 	struct typec_capability typec_cap;
 };
 
+#define TPS_MAX_LEN	sizeof(u64)

That is not big enough. The registers of this chip can be as big as 64 bytes. The identity register alone is 25 bytes, so the above would make the driver fail quite fast. Can you set the maximum to 64?

#define TPS_MAX_LEN 64

drivers/usb/typec/tps6598x.c struct tps6598x { · line 96

Quoted 12 lines

drivers/usb/typec/tps6598x.c +4 -1

 static int
 tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 {
-	u8 data[len + 1];
+	u8 data[TPS_MAX_LEN + 1];
 	int ret;
 
+	if (WARN_ON(len + 1 > sizeof(data)))
+		return -EINVAL;
+
 	if (!tps->i2c_protocol)
 		return regmap_raw_read(tps->regmap, reg, val, len);

Thanks,

Kees Cook

author

2018-06-21 20:31 UTC #2 lore raw

Quoted 19 lines · Heikki Krogerus

Quoted 18 lines · Kees Cook
In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum buffer size and adds a sanity check.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/typec/tps6598x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/typec/tps6598x.c +0 -0
index 4b4c8d271b27..396193f85e6d 100644
@@ -81,12 +81,17 @@ struct tps6598x {
      struct typec_capability typec_cap;
 };
 

Quoted 7 lines

drivers/usb/typec/tps6598x.c struct tps6598x { · line 84
Quoted 1 line
drivers/usb/typec/tps6598x.c +1 -0
+#define TPS_MAX_LEN  sizeof(u64)
That is not big enough. The registers of this chip can be as big as 64 bytes. The identity register alone is 25 bytes, so the above would make the driver fail quite fast. Can you set the maximum to 64?
#define TPS_MAX_LEN 64

Oops! Yes, thanks, I missed this usage:

        struct tps6598x_rx_identity_reg id;
...
        ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
                                  &id, sizeof(id));

struct tps6598x_rx_identity_reg {

        u8                         status;               /*     0     1 */
        struct usb_pd_identity     identity;             /*     1    12 */
        u32                        vdo[3];               /*    13    12 */

        /* size: 25, cachelines: 1, members: 3 */
        /* last cacheline: 25 bytes */
};

But that's still only 25 bytes. Where is the 64? I see these:

   return tps6598x_block_read(tps, reg, val, sizeof(u16));
   return tps6598x_block_read(tps, reg, val, sizeof(u32));
   return tps6598x_block_read(tps, reg, val, sizeof(u64));
   ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
                             &id, sizeof(id));
           ret = tps6598x_block_read(tps, TPS_REG_DATA1,
                                     out_data, out_len);
           ret = tps6598x_block_read(tps, TPS_REG_DATA1, &val, sizeof(u8));

1, 2, 4, 8, 25, and "out_len" in tps6598x_exec_cmd(), which is always zero:

   ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);
   ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);

I clearly need a v2 of this patch, but I just want to make sure I get the right value. :)

Thanks for the review!

-Kees

Heikki Krogerus

maintainer

2018-06-25 14:44 UTC #3 lore raw

Quoted 56 lines · Kees Cook

Quoted 19 lines · Heikki Krogerus
Quoted 18 lines · Kees Cook
In the quest to remove all stack VLA usage from the kernel[1], this uses the maximum buffer size and adds a sanity check.
[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
Signed-off-by Kees Cook <[email protected]>
---
 drivers/usb/typec/tps6598x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/typec/tps6598x.c +0 -0
index 4b4c8d271b27..396193f85e6d 100644
@@ -81,12 +81,17 @@ struct tps6598x {
      struct typec_capability typec_cap;
 };
 
Quoted 7 lines
drivers/usb/typec/tps6598x.c struct tps6598x { · line 84
Quoted 1 line
drivers/usb/typec/tps6598x.c +1 -0
+#define TPS_MAX_LEN  sizeof(u64)
That is not big enough. The registers of this chip can be as big as 64 bytes. The identity register alone is 25 bytes, so the above would make the driver fail quite fast. Can you set the maximum to 64?
#define TPS_MAX_LEN 64
Oops! Yes, thanks, I missed this usage:
        struct tps6598x_rx_identity_reg id;
...
        ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
                                  &id, sizeof(id));
struct tps6598x_rx_identity_reg {
        u8                         status;               /*     0     1 */
        struct usb_pd_identity     identity;             /*     1    12 */
        u32                        vdo[3];               /*    13    12 */
        /* size: 25, cachelines: 1, members: 3 */
        /* last cacheline: 25 bytes */
};
But that's still only 25 bytes. Where is the 64? I see these:
   return tps6598x_block_read(tps, reg, val, sizeof(u16));
   return tps6598x_block_read(tps, reg, val, sizeof(u32));
   return tps6598x_block_read(tps, reg, val, sizeof(u64));
   ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
                             &id, sizeof(id));
           ret = tps6598x_block_read(tps, TPS_REG_DATA1,
                                     out_data, out_len);
           ret = tps6598x_block_read(tps, TPS_REG_DATA1, &val, sizeof(u8));

Quoted 7 lines

1, 2, 4, 8, 25, and "out_len" in tps6598x_exec_cmd(), which is always zero:
   ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);
   ret = tps6598x_exec_cmd(tps, cmd, 0, NULL, 0, NULL);
I clearly need a v2 of this patch, but I just want to make sure I get the right value. :)

64 is the max No. Data Bytes at least for the Data1 and Data2 registers, and it seems for few other registers as well [1] (ch. 1.3.2). But I don't mind if you still prefer to use 25 as the value. I'm happy as long as we don't break the driver :-)

[1] http://www.ti.com/lit/ug/slvuan1a/slvuan1a.pdf

Thanks,