diff mbox series

[v2] iio: sx9324: avoid copying property strings

Message ID 20231026-strncpy-drivers-iio-proximity-sx9324-c-v2-1-cee6e5db700c@google.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] iio: sx9324: avoid copying property strings | expand

Commit Message

Justin Stitt Oct. 26, 2023, 11:53 p.m. UTC 
We're doing some needless string copies when trying to assign the proper
`prop` string. We can make `prop` a const char* and simply assign to
string literals.

For the case where a format string is used, let's allocate some memory
via kasprintf() and point prop to it.

This also cleans up some deprecated strncpy() uses [1].

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v2:
- make prop a const char* and do simple assignments (thanks Jonathan)
- rebase onto 3a568e3a961ba330
- Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1e7c@google.com
---
Note: build-tested
---
 drivers/iio/proximity/sx9324.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)


---
base-commit: 3a568e3a961ba330091cd031647e4c303fa0badb
change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Oct. 27, 2023, 4:18 p.m. UTC | #1 
On Thu, Oct 26, 2023 at 11:53:28PM +0000, Justin Stitt wrote:
> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.
> 
> For the case where a format string is used, let's allocate some memory
> via kasprintf() and point prop to it.
> 
> This also cleans up some deprecated strncpy() uses [1].
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Looks like a reasonable way to go.

Reviewed-by: Kees Cook <keescook@chromium.org>
Jonathan Cameron Oct. 28, 2023, 3:26 p.m. UTC | #2 
On Thu, 26 Oct 2023 23:53:28 +0000
Justin Stitt <justinstitt@google.com> wrote:

> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.
> 
> For the case where a format string is used, let's allocate some memory
> via kasprintf() and point prop to it.
> 
> This also cleans up some deprecated strncpy() uses [1].
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Seems reasonable to me.

+CC Gwendal (+ Stephen) as it's Gwendal's driver and I think they are still actively
maintaining it.

> ---
> Changes in v2:
> - make prop a const char* and do simple assignments (thanks Jonathan)
> - rebase onto 3a568e3a961ba330
> - Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1e7c@google.com
> ---
> Note: build-tested
> ---
>  drivers/iio/proximity/sx9324.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index 438f9c9aba6e..c8547035cb47 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -885,7 +885,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
>  #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
>  	unsigned int pin_defs[SX9324_NUM_PINS];
> -	char prop[] = SX9324_PROXRAW_DEF;
> +	const char *prop = SX9324_PROXRAW_DEF;
>  	u32 start = 0, raw = 0, pos = 0;
>  	int ret, count, ph, pin;
>  	const char *res;
> @@ -899,7 +899,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  	case SX9324_REG_AFE_PH2:
>  	case SX9324_REG_AFE_PH3:
>  		ph = reg_def->reg - SX9324_REG_AFE_PH0;
> -		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
> +		prop = kasprintf(GFP_KERNEL, "semtech,ph%d-pin", ph);
>  
>  		count = device_property_count_u32(dev, prop);
>  		if (count != ARRAY_SIZE(pin_defs))
> @@ -913,6 +913,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  			raw |= (pin_defs[pin] << (2 * pin)) &
>  			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
>  		reg_def->def = raw;
> +		kfree(prop);
>  		break;
>  	case SX9324_REG_AFE_CTRL0:
>  		ret = device_property_read_string(dev,
> @@ -937,11 +938,9 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  	case SX9324_REG_AFE_CTRL4:
>  	case SX9324_REG_AFE_CTRL7:
>  		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
> -			strncpy(prop, "semtech,ph01-resolution",
> -				ARRAY_SIZE(prop));
> +			prop = "semtech,ph01-resolution";
>  		else
> -			strncpy(prop, "semtech,ph23-resolution",
> -				ARRAY_SIZE(prop));
> +			prop = "semtech,ph23-resolution";
>  
>  		ret = device_property_read_u32(dev, prop, &raw);
>  		if (ret)
> @@ -1012,11 +1011,9 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  	case SX9324_REG_PROX_CTRL0:
>  	case SX9324_REG_PROX_CTRL1:
>  		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
> -			strncpy(prop, "semtech,ph01-proxraw-strength",
> -				ARRAY_SIZE(prop));
> +			prop = "semtech,ph01-proxraw-strength";
>  		else
> -			strncpy(prop, "semtech,ph23-proxraw-strength",
> -				ARRAY_SIZE(prop));
> +			prop = "semtech,ph23-proxraw-strength";
>  		ret = device_property_read_u32(dev, prop, &raw);
>  		if (ret)
>  			break;
> 
> ---
> base-commit: 3a568e3a961ba330091cd031647e4c303fa0badb
> change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039
> 
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>
Stephen Boyd Oct. 30, 2023, 9:44 p.m. UTC | #3 
Quoting Jonathan Cameron (2023-10-28 08:26:38)
> On Thu, 26 Oct 2023 23:53:28 +0000
> Justin Stitt <justinstitt@google.com> wrote:
>
> > We're doing some needless string copies when trying to assign the proper
> > `prop` string. We can make `prop` a const char* and simply assign to
> > string literals.
> >
> > For the case where a format string is used, let's allocate some memory
> > via kasprintf() and point prop to it.
> >
> > This also cleans up some deprecated strncpy() uses [1].
> >
> > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> > Link: https://github.com/KSPP/linux/issues/90
> > Cc: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
>
> Seems reasonable to me.
>
> +CC Gwendal (+ Stephen) as it's Gwendal's driver and I think they are still actively
> maintaining it.

Thanks! I have some review comments.

> > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> > index 438f9c9aba6e..c8547035cb47 100644
> > --- a/drivers/iio/proximity/sx9324.c
> > +++ b/drivers/iio/proximity/sx9324.c
> > @@ -885,7 +885,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
> >  #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
> >  #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
> >       unsigned int pin_defs[SX9324_NUM_PINS];
> > -     char prop[] = SX9324_PROXRAW_DEF;
> > +     const char *prop = SX9324_PROXRAW_DEF;

Do we need this define anymore, or the initialization?

> >       u32 start = 0, raw = 0, pos = 0;
> >       int ret, count, ph, pin;
> >       const char *res;
> > @@ -899,7 +899,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
> >       case SX9324_REG_AFE_PH2:
> >       case SX9324_REG_AFE_PH3:
> >               ph = reg_def->reg - SX9324_REG_AFE_PH0;
> > -             snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
> > +             prop = kasprintf(GFP_KERNEL, "semtech,ph%d-pin", ph);

Do we not care if the allocation fails? We just use the default?

> >
> >               count = device_property_count_u32(dev, prop);
> >               if (count != ARRAY_SIZE(pin_defs))
> > @@ -913,6 +913,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
> >                       raw |= (pin_defs[pin] << (2 * pin)) &
> >                              SX9324_REG_AFE_PH0_PIN_MASK(pin);
> >               reg_def->def = raw;
> > +             kfree(prop);

We need to free it in other places too, like if the count doesn't match.
It may be easier to extract this section and just have 4 string
literals.

	switch (reg_def->reg) {
        case SX9324_REG_AFE_PH0:
		reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph0-pin");
		break;
        case SX9324_REG_AFE_PH1:
		reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph1-pin");
		break;
        case SX9324_REG_AFE_PH2:
		reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph2-pin");
		break;
        case SX9324_REG_AFE_PH3:
		reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph3-pin");
		break;

> >       case SX9324_REG_AFE_CTRL0:
> >               ret = device_property_read_string(dev,
Justin Stitt Dec. 12, 2023, 12:46 a.m. UTC | #4 
Hi,

On Mon, Oct 30, 2023 at 2:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
>
> We need to free it in other places too, like if the count doesn't match.
> It may be easier to extract this section and just have 4 string
> literals.
>
>         switch (reg_def->reg) {
>         case SX9324_REG_AFE_PH0:
>                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph0-pin");
>                 break;
>         case SX9324_REG_AFE_PH1:
>                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph1-pin");
>                 break;
>         case SX9324_REG_AFE_PH2:
>                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph2-pin");
>                 break;
>         case SX9324_REG_AFE_PH3:
>                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph3-pin");
>                 break;
>

I've submitted v3 of this patch [1] trying out Stephen's idea. I'd
appreciate feedback.

[1]: https://lore.kernel.org/all/20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8a5d@google.com/

Thanks
Justin
Gwendal Grignou Dec. 12, 2023, 11:51 p.m. UTC | #5 
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

On Mon, Dec 11, 2023 at 4:46 PM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi,
>
> On Mon, Oct 30, 2023 at 2:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> >
> > We need to free it in other places too, like if the count doesn't match.
> > It may be easier to extract this section and just have 4 string
> > literals.
> >
> >         switch (reg_def->reg) {
> >         case SX9324_REG_AFE_PH0:
> >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph0-pin");
> >                 break;
> >         case SX9324_REG_AFE_PH1:
> >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph1-pin");
> >                 break;
> >         case SX9324_REG_AFE_PH2:
> >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph2-pin");
> >                 break;
> >         case SX9324_REG_AFE_PH3:
> >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph3-pin");
> >                 break;
> >
>
> I've submitted v3 of this patch [1] trying out Stephen's idea. I'd
> appreciate feedback.
>
> [1]: https://lore.kernel.org/all/20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8a5d@google.com/
>
> Thanks
> Justin
Jonathan Cameron Dec. 17, 2023, 1 p.m. UTC | #6 
On Tue, 12 Dec 2023 15:51:04 -0800
Gwendal Grignou <gwendal@chromium.org> wrote:

> Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Hi Gwendal

I'll ignore this tag given the email you've replied to says there is a different
implementation. Please take a look at that version instead.

Jonathan

> 
> On Mon, Dec 11, 2023 at 4:46 PM Justin Stitt <justinstitt@google.com> wrote:
> >
> > Hi,
> >
> > On Mon, Oct 30, 2023 at 2:44 PM Stephen Boyd <swboyd@chromium.org> wrote:  
> > >
> > >
> > > We need to free it in other places too, like if the count doesn't match.
> > > It may be easier to extract this section and just have 4 string
> > > literals.
> > >
> > >         switch (reg_def->reg) {
> > >         case SX9324_REG_AFE_PH0:
> > >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph0-pin");
> > >                 break;
> > >         case SX9324_REG_AFE_PH1:
> > >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph1-pin");
> > >                 break;
> > >         case SX9324_REG_AFE_PH2:
> > >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph2-pin");
> > >                 break;
> > >         case SX9324_REG_AFE_PH3:
> > >                 reg_def = sx9324_parse_phase_prop(dev, reg_def, "semtech,ph3-pin");
> > >                 break;
> > >  
> >
> > I've submitted v3 of this patch [1] trying out Stephen's idea. I'd
> > appreciate feedback.
> >
> > [1]: https://lore.kernel.org/all/20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8a5d@google.com/
> >
> > Thanks
> > Justin
diff mbox series

Patch

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 438f9c9aba6e..c8547035cb47 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -885,7 +885,7 @@  sx9324_get_default_reg(struct device *dev, int idx,
 #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
 #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
 	unsigned int pin_defs[SX9324_NUM_PINS];
-	char prop[] = SX9324_PROXRAW_DEF;
+	const char *prop = SX9324_PROXRAW_DEF;
 	u32 start = 0, raw = 0, pos = 0;
 	int ret, count, ph, pin;
 	const char *res;
@@ -899,7 +899,7 @@  sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_AFE_PH2:
 	case SX9324_REG_AFE_PH3:
 		ph = reg_def->reg - SX9324_REG_AFE_PH0;
-		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
+		prop = kasprintf(GFP_KERNEL, "semtech,ph%d-pin", ph);
 
 		count = device_property_count_u32(dev, prop);
 		if (count != ARRAY_SIZE(pin_defs))
@@ -913,6 +913,7 @@  sx9324_get_default_reg(struct device *dev, int idx,
 			raw |= (pin_defs[pin] << (2 * pin)) &
 			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
 		reg_def->def = raw;
+		kfree(prop);
 		break;
 	case SX9324_REG_AFE_CTRL0:
 		ret = device_property_read_string(dev,
@@ -937,11 +938,9 @@  sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_AFE_CTRL4:
 	case SX9324_REG_AFE_CTRL7:
 		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
-			strncpy(prop, "semtech,ph01-resolution",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph01-resolution";
 		else
-			strncpy(prop, "semtech,ph23-resolution",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph23-resolution";
 
 		ret = device_property_read_u32(dev, prop, &raw);
 		if (ret)
@@ -1012,11 +1011,9 @@  sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_PROX_CTRL0:
 	case SX9324_REG_PROX_CTRL1:
 		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
-			strncpy(prop, "semtech,ph01-proxraw-strength",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph01-proxraw-strength";
 		else
-			strncpy(prop, "semtech,ph23-proxraw-strength",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph23-proxraw-strength";
 		ret = device_property_read_u32(dev, prop, &raw);
 		if (ret)
 			break;