[PATCH 1/2] usb: typec: tipd: Don't read/write more bytes than required

[PATCH 1/2] usb: typec: tipd: Don't read/write more bytes than required

[Sven Peter]

tps6598x_block_read/write always read 65 bytes of data even when much
less is required when I2C_FUNC_I2C is used. Reduce this to the correct
number.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 21b3ae25c76d..c18ec3785592 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -123,7 +123,7 @@ tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, size_t len)
 	if (!tps->i2c_protocol)
 		return regmap_raw_read(tps->regmap, reg, val, len);
 
-	ret = regmap_raw_read(tps->regmap, reg, data, sizeof(data));
+	ret = regmap_raw_read(tps->regmap, reg, data, len + 1);
 	if (ret)
 		return ret;
 
@@ -145,7 +145,7 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
 	data[0] = len;
 	memcpy(&data[1], val, len);
 
-	return regmap_raw_write(tps->regmap, reg, data, sizeof(data));
+	return regmap_raw_write(tps->regmap, reg, data, len + 1);
 }
 
 static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
-- 
2.25.1

[PATCH 2/2] usb: typec: tipd: Add an additional overflow check

[Sven Peter]

tps6598x_block_read already checks for the maximum length of the read
but tps6598x_block_write does not. Add the symmetric check there as
well.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/usb/typec/tipd/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index c18ec3785592..70e2d0d410c9 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -139,6 +139,9 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
 {
 	u8 data[TPS_MAX_LEN + 1];
 
+	if (WARN_ON(len + 1 > sizeof(data)))
+		return -EINVAL;
+
 	if (!tps->i2c_protocol)
 		return regmap_raw_write(tps->regmap, reg, val, len);
 
-- 
2.25.1

Re: [PATCH 2/2] usb: typec: tipd: Add an additional overflow check

[Greg Kroah-Hartman]

On Tue, Sep 14, 2021 at 12:42:53PM +0200, Sven Peter wrote:
> tps6598x_block_read already checks for the maximum length of the read
> but tps6598x_block_write does not. Add the symmetric check there as
> well.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  drivers/usb/typec/tipd/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index c18ec3785592..70e2d0d410c9 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -139,6 +139,9 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
>  {
>  	u8 data[TPS_MAX_LEN + 1];
>  
> +	if (WARN_ON(len + 1 > sizeof(data)))
> +		return -EINVAL;

No need to crash anything.  If this is a valid thing for us to check,
let's check it and handle the error, but we should not reboot systems
that are running with panic-on-warn enabled, right?

thanks,

greg k-h

Re: [PATCH 2/2] usb: typec: tipd: Add an additional overflow check

[Sven Peter]

On Tue, Sep 14, 2021, at 13:00, Greg Kroah-Hartman wrote:
> On Tue, Sep 14, 2021 at 12:42:53PM +0200, Sven Peter wrote:
> > tps6598x_block_read already checks for the maximum length of the read
> > but tps6598x_block_write does not. Add the symmetric check there as
> > well.
> > 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > ---
> >  drivers/usb/typec/tipd/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index c18ec3785592..70e2d0d410c9 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -139,6 +139,9 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> >  {
> >  	u8 data[TPS_MAX_LEN + 1];
> >  
> > +	if (WARN_ON(len + 1 > sizeof(data)))
> > +		return -EINVAL;
> 
> No need to crash anything.  If this is a valid thing for us to check,
> let's check it and handle the error, but we should not reboot systems
> that are running with panic-on-warn enabled, right?

Sure, that makes sense. I guess the same point applies to the WARN_ON in
the same check in tps6598x_block_read. I can add a patch to remove that one
to v2 as well.


thanks,


Sven

Re: [PATCH 2/2] usb: typec: tipd: Add an additional overflow check

[Greg Kroah-Hartman]

On Tue, Sep 14, 2021 at 01:18:16PM +0200, Sven Peter wrote:
> 
> 
> On Tue, Sep 14, 2021, at 13:00, Greg Kroah-Hartman wrote:
> > On Tue, Sep 14, 2021 at 12:42:53PM +0200, Sven Peter wrote:
> > > tps6598x_block_read already checks for the maximum length of the read
> > > but tps6598x_block_write does not. Add the symmetric check there as
> > > well.
> > > 
> > > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > > ---
> > >  drivers/usb/typec/tipd/core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > > index c18ec3785592..70e2d0d410c9 100644
> > > --- a/drivers/usb/typec/tipd/core.c
> > > +++ b/drivers/usb/typec/tipd/core.c
> > > @@ -139,6 +139,9 @@ static int tps6598x_block_write(struct tps6598x *tps, u8 reg,
> > >  {
> > >  	u8 data[TPS_MAX_LEN + 1];
> > >  
> > > +	if (WARN_ON(len + 1 > sizeof(data)))
> > > +		return -EINVAL;
> > 
> > No need to crash anything.  If this is a valid thing for us to check,
> > let's check it and handle the error, but we should not reboot systems
> > that are running with panic-on-warn enabled, right?
> 
> Sure, that makes sense. I guess the same point applies to the WARN_ON in
> the same check in tps6598x_block_read. I can add a patch to remove that one
> to v2 as well.

That would be great, thanks.

greg k-h