| Message ID | 20180620182840.GA24775@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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
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 --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);
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(-)