Re: [PATCH] usb: typec: tps6598x: Remove VLA usage

[Heikki Krogerus]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
> >>
> >> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> >> ---
> >>  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,

-- 
heikki