linux-hardening - Re: [PATCH v2] iio: sx9324: avoid copying property strings

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: Sat, 28 Oct 2023 16:26:38 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org, Gwendal
 Grignou <gwendal@...omium.org>, Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH v2] iio: sx9324: avoid copying property strings

On Thu, 26 Oct 2023 23:53:28 +0000
Justin Stitt <justinstitt@...gle.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@...r.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@...gle.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@...gle.com>
>