On Thu, 21 Sep 2023 07:01:01 +0000 Justin Stitt <justinstitt@xxxxxxxxxx> wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > `prop` is defined as this string literal with size 30 (including null): > | #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength" > | char prop[] = SX9324_PROXRAW_DEF; > > Each of the strncpy->strscpy replacements involve string literals with a > size less than 30 which means there are no current problems with how > strncpy is used. However, let's move away from using strncpy entirely. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding. > > Moreover, let's opt for the more conventional `sizeof()` as opposed to > `ARRAY_SIZE` for these simple strings. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@xxxxxxxxxxxxxxx > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > --- > FWIW: It seems fragile to base future `prop` stores on the > size of it's default string. Agreed - can we just get rid of the copying? Just set a const char * to point to appropriate string instead and use that? The dance is just about reasonable for the case where there is a string format being used to fill in the numbers, here I can't see any advantage at all. (for the other case, a kasprintf() or similar is probably more appropriate anyway) given this isn't a particular hot path. Jonathan > > Note: build-tested > --- > drivers/iio/proximity/sx9324.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c > index 438f9c9aba6e..25ac2733bcef 100644 > --- a/drivers/iio/proximity/sx9324.c > +++ b/drivers/iio/proximity/sx9324.c > @@ -937,11 +937,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)); > + strscpy(prop, "semtech,ph01-resolution", sizeof(prop)); > else > - strncpy(prop, "semtech,ph23-resolution", > - ARRAY_SIZE(prop)); > + strscpy(prop, "semtech,ph23-resolution", sizeof(prop)); > > ret = device_property_read_u32(dev, prop, &raw); > if (ret) > @@ -1012,11 +1010,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)); > + strscpy(prop, "semtech,ph01-proxraw-strength", sizeof(prop)); > else > - strncpy(prop, "semtech,ph23-proxraw-strength", > - ARRAY_SIZE(prop)); > + strscpy(prop, "semtech,ph23-proxraw-strength", sizeof(prop)); > ret = device_property_read_u32(dev, prop, &raw); > if (ret) > break; > > --- > base-commit: 2cf0f715623872823a72e451243bbf555d10d032 > change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039 > > Best regards, > -- > Justin Stitt <justinstitt@xxxxxxxxxx> >
