[PATCH v1] can: mcp251xfd: Add some sysfs debug interfaces for registers r/w

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

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

On 1/22/21 7:22 AM, Su Yanjun wrote:

When i debug mcp2518fd, some method to track registers is
needed. This easy debug interface will be ok.

NACK

As the driver uses regmap, everything should be there already.

To read use:

| cat /sys/kernel/debug/regmap/spi0.0-crc/registers

Register write support for devices that are handles by proper kernel drivers is
a pure debugging tool, thus not enabled by default, not even with a Kconfig
switch. You have to enable it manually, have a look at commit:

09c6ecd39410 regmap: Add support for writing to regmap registers via debugfs

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

在 2021/1/22 下午3:22, Marc Kleine-Budde 写道:

On 1/22/21 7:22 AM, Su Yanjun wrote:

quoted

When i debug mcp2518fd, some method to track registers is
needed. This easy debug interface will be ok.

NACK

As the driver uses regmap, everything should be there already.

To read use:

| cat /sys/kernel/debug/regmap/spi0.0-crc/registers

Register write support for devices that are handles by proper kernel drivers is
a pure debugging tool, thus not enabled by default, not even with a Kconfig
switch. You have to enable it manually, have a look at commit:

09c6ecd39410 regmap: Add support for writing to regmap registers via debugfs

You're right.

Thank you

regards,
Marc

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.

Vincent MAILHOL [off-list ref] 于2021年1月22日周五 下午4:51写道：

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:

quoted

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 check the input buffer in __get_param with 16 bytes limit.
Thanks for your review.

I see no checks of the buf len in your code and I suspect it to be
vulnerable to stack buffer overflow exploits.

quoted

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