On Sat, Aug 14, 2021 at 12:06 PM Len Baker <len.baker@xxxxxxx> wrote: > > strcpy() performs no bounds checking on the destination buffer. This > could result in linear overflows beyond the end of the buffer, leading > to all kinds of misbehaviors. So, remove all the uses and add > devm_kstrdup() or devm_kasprintf() instead. > > This patch is an effort to clean up the proliferation of str*() > functions in the kernel and a previous step in the path to remove > the strcpy function from the kernel entirely [1]. > > [1] https://github.com/KSPP/linux/issues/88 Thanks for an update, my comments below. ... > This patch doesn't change the logic. I think it is better to use the > current logic and not use always the plus and minus signs as suggested > in the previous version. I don't like the idea that 0 has sign. Agree on that, the safest way to go with. ... > const char *orient; > char *str; > int i; > + struct device *dev; Please, keep this in reversed xmas tree order (longer lines first). ... > + dev = regmap_get_device(st->map); I haven't checked the code in between, but maybe it's possible to move an assignment directly to the definition block above. ... > + /* > + * The value is inverted according to the following "to one of the" And technically speaking "inversion" is not the same as negation (which is "sign inversion"). > + * rules: > + * > + * 1) Drop leading minus. > + * 2) Add leading minus. > + * 3) Leave 0 as is. > + */ > + if (orient[0] == '-') > + str = devm_kstrdup(dev, orient + 1, GFP_KERNEL); > + else if (orient[0] != '0' || orient[1] != '\0') > + str = devm_kasprintf(dev, GFP_KERNEL, "-%s", orient); I would go with the logic I suggested later on, i.e. else if (orient[0] == '0' && orient[1] == '\0') str = devm_kstrdup(dev, orient, GFP_KERNEL); and below changed accordingly. It will clarify the "0" check. > + else > + str = devm_kstrdup(dev, orient, GFP_KERNEL); > + Redundant blank line. > + if (!str) > return -ENOMEM; -- With Best Regards, Andy Shevchenko
