linux-kernel - Re: [PATCH] usb: typec: tps6598x: Remove VLA usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Jun 2018 13:31:23 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-usb@...r.kernel.org
Subject: Re: [PATCH] usb: typec: tps6598x: Remove VLA usage

On Thu, Jun 21, 2018 at 3:00 AM, Heikki Krogerus
<heikki.krogerus@...ux.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@...omium.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

-- 
Kees Cook
Pixel Security