Re: [PATCH v1] can: mcp251xfd: Add some sysfs debug interfaces for registers r/w
From: Vincent MAILHOL <hidden>
Date: 2021-01-22 08:53:47
Also in:
lkml, netdev
Hi, In addition to Marc’s comment, I also have security concerns. On Fri. 22 Jan 2021 at 15:22, Su Yanjun [off-list ref] wrote:
When i debug mcp2518fd, some method to track registers is needed. This easy debug interface will be ok. For example, read a register at 0xe00: echo 0xe00 > can_get_reg cat can_get_reg write a register at 0xe00: echo 0xe00,0x60 > can_set_reg
What about:
printf "A%0.s" {1..1000} > can_set_reg
Doesn’t it crash the kernel?
I see no checks of the buf len in your code and I suspect it to be
vulnerable to stack buffer overflow exploits.
quoted hunk ↗ jump to hunk
Signed-off-by: Su Yanjun <redacted> --- .../net/can/spi/mcp251xfd/mcp251xfd-core.c | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+)diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c index ab8aad0a7594..d65abe5505d5 100644 --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c@@ -27,6 +27,131 @@ #define DEVICE_NAME "mcp251xfd" +/* Add sysfs debug interface for easy to debug + * + * For example, + * + * - read a register + * echo 0xe00 > can_get_reg + * cat can_get_reg + * + * - write a register + * echo 0xe00,0x1 > can_set_reg + * + */ +static int reg_offset; + +static int __get_param(const char *buf, char *off, char *val) +{ + int len; + + if (!buf || !off || !val) + return -EINVAL; + + len = 0; + while (*buf != ',') { + *off++ = *buf++; + len++; + + if (len >= 16) + return -EINVAL; + } + + buf++; + + *off = '\0'; + + len = 0; + while (*buf) { + *val++ = *buf++; + len++; + + if (len >= 16) + return -EINVAL; + } + + *val = '\0'; + + return 0; +} + +static ssize_t can_get_reg_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + int err; + u32 val; + struct mcp251xfd_priv *priv; + + priv = dev_get_drvdata(dev); + + err = regmap_read(priv->map_reg, reg_offset, &val); + if (err) + return 0; + + return sprintf(buf, "reg = 0x%08x, val = 0x%08x\n", reg_offset, val); +} + +static ssize_t can_get_reg_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t len) +{ + u32 off; + + reg_offset = 0; + + if (kstrtouint(buf, 0, &off) || (off % 4)) + return -EINVAL; + + reg_offset = off; + + return len; +} + +static ssize_t can_set_reg_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return 0; +} + +static ssize_t can_set_reg_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t len) +{ + struct mcp251xfd_priv *priv; + u32 off, val; + int err; + + char s1[16]; + char s2[16]; + + if (__get_param(buf, s1, s2)) + return -EINVAL; + + if (kstrtouint(s1, 0, &off) || (off % 4)) + return -EINVAL; + + if (kstrtouint(s2, 0, &val)) + return -EINVAL; + + err = regmap_write(priv->map_reg, off, val); + if (err) + return -EINVAL; + + return len; +} + +static DEVICE_ATTR_RW(can_get_reg); +static DEVICE_ATTR_RW(can_set_reg); + +static struct attribute *can_attributes[] = { + &dev_attr_can_get_reg.attr, + &dev_attr_can_set_reg.attr, + NULL +}; + +static const struct attribute_group can_group = { + .attrs = can_attributes, + NULL +}; + static const struct mcp251xfd_devtype_data mcp251xfd_devtype_data_mcp2517fd = { .quirks = MCP251XFD_QUIRK_MAB_NO_WARN | MCP251XFD_QUIRK_CRC_REG | MCP251XFD_QUIRK_CRC_RX | MCP251XFD_QUIRK_CRC_TX |@@ -2944,6 +3069,12 @@ static int mcp251xfd_probe(struct spi_device *spi) if (err) goto out_free_candev; + err = sysfs_create_group(&spi->dev.kobj, &can_group); + if (err) { + netdev_err(priv->ndev, "Create can group fail.\n"); + goto out_free_candev; + } + err = can_rx_offload_add_manual(ndev, &priv->offload, MCP251XFD_NAPI_WEIGHT); if (err)@@ -2972,6 +3103,7 @@ static int mcp251xfd_remove(struct spi_device *spi) mcp251xfd_unregister(priv); spi->max_speed_hz = priv->spi_max_speed_hz_orig; free_candev(ndev); + sysfs_remove_group(&spi->dev.kobj, &can_group); return 0; } --2.25.1