* [PATCH] usb: typec: tps6598x: Remove VLA usage @ 2018-06-20 18:28 Kees Cook 2018-06-21 10:00 ` Heikki Krogerus 0 siblings, 1 reply; 4+ messages in thread From: Kees Cook @ 2018-06-20 18:28 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Heikki Krogerus, linux-kernel, linux-usb 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) + 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); -- 2.17.1 -- Kees Cook Pixel Security ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tps6598x: Remove VLA usage 2018-06-20 18:28 [PATCH] usb: typec: tps6598x: Remove VLA usage Kees Cook @ 2018-06-21 10:00 ` Heikki Krogerus 2018-06-21 20:31 ` Kees Cook 0 siblings, 1 reply; 4+ messages in thread From: Heikki Krogerus @ 2018-06-21 10:00 UTC (permalink / raw) To: Kees Cook; +Cc: Greg Kroah-Hartman, linux-kernel, linux-usb 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, -- heikki ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tps6598x: Remove VLA usage 2018-06-21 10:00 ` Heikki Krogerus @ 2018-06-21 20:31 ` Kees Cook 2018-06-25 14:44 ` Heikki Krogerus 0 siblings, 1 reply; 4+ messages in thread From: Kees Cook @ 2018-06-21 20:31 UTC (permalink / raw) To: Heikki Krogerus; +Cc: Greg Kroah-Hartman, LKML, linux-usb 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 -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: typec: tps6598x: Remove VLA usage 2018-06-21 20:31 ` Kees Cook @ 2018-06-25 14:44 ` Heikki Krogerus 0 siblings, 0 replies; 4+ messages in thread From: Heikki Krogerus @ 2018-06-25 14:44 UTC (permalink / raw) To: Kees Cook; +Cc: Greg Kroah-Hartman, LKML, linux-usb 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, -- heikki ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-25 14:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-20 18:28 [PATCH] usb: typec: tps6598x: Remove VLA usage Kees Cook 2018-06-21 10:00 ` Heikki Krogerus 2018-06-21 20:31 ` Kees Cook 2018-06-25 14:44 ` Heikki Krogerus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).