| Message ID | 20220316150533.421349-1-krzysztof.kozlowski@canonical.com |
|---|---|
| Headers | show |
| Series | Fix broken usage of driver_override (and kfree of static memory) | expand |
On 16/03/2022 16:54, Andy Shevchenko wrote: > On Wed, Mar 16, 2022 at 5:06 PM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: > > ... > >> +int driver_set_override(struct device *dev, const char **override, >> + const char *s, size_t len) >> +{ >> + const char *new, *old; >> + char *cp; I focused on some other topics, so I did not respond to your questions for some time. > >> + if (!dev || !override || !s) >> + return -EINVAL; > > Sorry, I didn't pay much attention on this. First of all, I would drop > dev checks and simply require that dev should be valid. Do you expect > this can be called when dev is invalid? No, I can skip the check. > I would like to hear if it's > anything but theoretical. Second one, is the !s requirement. Do I > understand correctly that the string must be always present? But then > how we NULify the override? Is it possible? I did not change the convention of this sysfs hook, so removing of override is passing empty string "". Have in mind that his interface is mainly for sysfs, not for other drivers. > Third one is absence of > len check. See below. > >> + /* >> + * The stored value will be used in sysfs show callback (sysfs_emit()), >> + * which has a length limit of PAGE_SIZE and adds a trailing newline. >> + * Thus we can store one character less to avoid truncation during sysfs >> + * show. >> + */ >> + if (len >= (PAGE_SIZE - 1)) >> + return -EINVAL; > > I would relax this to make sure we can use it if \n is within this limit. Relax in what way? Store more than PAGE_SIZE? This is a user-visible string and the name of a driver. > >> + cp = strnchr(s, len, '\n'); >> + if (cp) >> + len = cp - s; >> + >> + new = kstrndup(s, len, GFP_KERNEL); > > Here is a word about the len check. > >> + if (!new) > > If len == 0, this won't trigger and you have something very > interesting as a result. True, empty string would be set as override. The API says that empty string clears the override, so len==0 should be valid (just like "\n" is ok). > > One way is to use ZERO_PTR_OR_NULL() another is explicitly check for 0 > and issue a (different?) error code. Best regards, Krzysztof
Hi, This is a continuation of my old patchset from 2019. [1] Back then, few drivers set driver_override wrong. I fixed Exynos in a different way after discussions. QCOM NGD was not fixed and a new user appeared - IMX SCU. It seems "char *" in driver_override looks too consty, so we tend to make a mistake of storing there string literals. Changes since latest v4 ======================= 1. Correct commit msgs and comments after Andy's review. 2. Re-order code in new helper (patch #1) (Andy). 3. Add tags. Changes since latest v3 ======================= 1. Wrap comments, extend comment in driver_set_override() about newline. 2. Minor commit msg fixes. 3. Add tags. Changes since latest v2 ======================= 1. Make all driver_override fields as "const char *", just like SPI and VDPA. (Mark) 2. Move "count" check to the new helper and add "count" argument. (Michael) 3. Fix typos in docs, patch subject. Extend doc. (Michael, Bjorn) 4. Compare pointers to reduce number of string readings in the helper. 5. Fix clk-imx return value. Changes since latest v1 (not the old 2019 solution): ==================================================== https://lore.kernel.org/all/708eabb1-7b35-d525-d4c3-451d4a3de84f@rasmusvillemoes.dk/ 1. Add helper for setting driver_override. 2. Use the helper. Dependencies (and stable): ========================== 1. All patches, including last three fixes, depend on the first patch introducing the helper. 2. The last three commits - fixes - are probably not backportable directly, because of this dependency. I don't know how to express this dependency here, since stable-kernel-rules.rst mentions only commits as possible dependencies. [1] https://lore.kernel.org/all/1550484960-2392-3-git-send-email-krzk@kernel.org/ Best regards, Krzysztof Krzysztof Kozlowski (11): driver: platform: Add helper for safer setting of driver_override amba: Use driver_set_override() instead of open-coding fsl-mc: Use driver_set_override() instead of open-coding hv: Use driver_set_override() instead of open-coding PCI: Use driver_set_override() instead of open-coding s390/cio: Use driver_set_override() instead of open-coding spi: Use helper for safer setting of driver_override vdpa: Use helper for safer setting of driver_override clk: imx: scu: Fix kfree() of static memory on setting driver_override slimbus: qcom-ngd: Fix kfree() of static memory on setting driver_override rpmsg: Fix kfree() of static memory on setting driver_override drivers/amba/bus.c | 28 +++-------------- drivers/base/driver.c | 56 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 28 +++-------------- drivers/bus/fsl-mc/fsl-mc-bus.c | 25 +++------------ drivers/clk/imx/clk-scu.c | 7 ++++- drivers/hv/vmbus_drv.c | 28 +++-------------- drivers/pci/pci-sysfs.c | 28 +++-------------- drivers/rpmsg/rpmsg_core.c | 3 +- drivers/rpmsg/rpmsg_internal.h | 11 +++++-- drivers/rpmsg/rpmsg_ns.c | 14 +++++++-- drivers/s390/cio/cio.h | 6 +++- drivers/s390/cio/css.c | 28 +++-------------- drivers/slimbus/qcom-ngd-ctrl.c | 13 +++++++- drivers/spi/spi.c | 26 +++------------ drivers/vdpa/vdpa.c | 29 +++-------------- include/linux/amba/bus.h | 6 +++- include/linux/device/driver.h | 2 ++ include/linux/fsl/mc.h | 6 ++-- include/linux/hyperv.h | 6 +++- include/linux/pci.h | 6 +++- include/linux/platform_device.h | 6 +++- include/linux/rpmsg.h | 6 ++-- include/linux/spi/spi.h | 2 ++ include/linux/vdpa.h | 4 ++- 24 files changed, 169 insertions(+), 205 deletions(-)