diff mbox

usb: typec: tps6598x: Remove VLA usage

Message ID 20180620182840.GA24775@beast (mailing list archive)
State New, archived
Headers show

Commit Message

Kees Cook June 20, 2018, 6:28 p.m. UTC
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 <keescook@chromium.org>
---
 drivers/usb/typec/tps6598x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Heikki Krogerus June 21, 2018, 10 a.m. UTC | #1
On Wed, Jun 20, 2018 at 11:28:40AM -0700, Kees Cook wrote:
> 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 <keescook@chromium.org>
> ---
>  drivers/usb/typec/tps6598x.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 4b4c8d271b27..396193f85e6d 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -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

>  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 June 21, 2018, 8:31 p.m. UTC | #2
On Thu, Jun 21, 2018 at 3:00 AM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Jun 20, 2018 at 11:28:40AM -0700, Kees Cook wrote:
>> 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 <keescook@chromium.org>
>> ---
>>  drivers/usb/typec/tps6598x.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
>> index 4b4c8d271b27..396193f85e6d 100644
>> --- a/drivers/usb/typec/tps6598x.c
>> +++ b/drivers/usb/typec/tps6598x.c
>> @@ -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

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 June 25, 2018, 2:44 p.m. UTC | #3
On Thu, Jun 21, 2018 at 01:31:23PM -0700, Kees Cook wrote:
> On Thu, Jun 21, 2018 at 3:00 AM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Wed, Jun 20, 2018 at 11:28:40AM -0700, Kees Cook wrote:
> >> 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 <keescook@chromium.org>
> >> ---
> >>  drivers/usb/typec/tps6598x.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> >> index 4b4c8d271b27..396193f85e6d 100644
> >> --- a/drivers/usb/typec/tps6598x.c
> >> +++ b/drivers/usb/typec/tps6598x.c
> >> @@ -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
> 
> 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. :)

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,
diff mbox

Patch

diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 4b4c8d271b27..396193f85e6d 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -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);