net/9p/trans_virtio.c: add null terminal for mount tag

PATCH Superseded piaojun 3 participants 7 messages Aug 2, 2018 – Aug 3, 2018 9P file system

Share & tools

piaojun

author

2018-08-02 01:49 UTC #0 lore raw

PATCH v2 1/1 net/9p/trans_virtio.c: add null terminal for mount tag

chan->tag is Non-null terminated which will result in printing messy code when debugging code. So we should add '\0' for tag to make the code more convenient and robust. In addition, I drop char->tag_len to simplify the code.

Signed-off-by Jun Piao <[email protected]>

---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

net/9p/trans_virtio.c +5 ~~-10~~

index d422bfc..0fe9c37 100644
@@ -89,10 +89,8 @@ struct virtio_chan {
 	unsigned long p9_max_pages;
 	/* Scatterlist: can be too big for stack. */
 	struct scatterlist sg[VIRTQUEUE_NUM];
-
-	int tag_len;
 	/*
-	 * tag name to identify a mount Non-null terminated
+	 * tag name to identify a mount null terminated
 	 */
 	char *tag;

@@ -529,10 +527,9 @@ static ssize_t p9_mount_tag_show(struct device *dev,
 	vdev = dev_to_virtio(dev);
 	chan = vdev->priv;
 
-	memcpy(buf, chan->tag, chan->tag_len);
-	buf[chan->tag_len] = 0;
+	memcpy(buf, chan->tag, strlen(chan->tag) + 1);
 
-	return chan->tag_len + 1;
+	return strlen(chan->tag) + 1;
 }
 
 static DEVICE_ATTR(mount_tag, 0444, p9_mount_tag_show, NULL);

@@ -585,7 +582,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kmalloc(tag_len, GFP_KERNEL);
+	tag = kzalloc(tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;

@@ -594,7 +591,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
 			   tag, tag_len);
 	chan->tag = tag;
-	chan->tag_len = tag_len;
 	err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
 	if (err) {
 		goto out_free_tag;

@@ -654,8 +650,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
 	mutex_lock(&virtio_9p_lock);
 	list_for_each_entry(chan, &virtio_chan_list, chan_list) {
-		if (!strncmp(devname, chan->tag, chan->tag_len) &&
-		    strlen(devname) == chan->tag_len) {
+		if (!strncmp(devname, chan->tag, strlen(chan->tag) + 1)) {
 			if (!chan->inuse) {
 				chan->inuse = true;
 				found = 1;

Dominique Martinet

maintainer

2018-08-02 01:54 UTC #1 lore raw

piaojun wrote on Thu, Aug 02, 2018:

Quoted 4 lines

chan->tag is Non-null terminated which will result in printing messy code when debugging code. So we should add '\0' for tag to make the code more convenient and robust. In addition, I drop char->tag_len to simplify the code.

Some new lines in commit message would be appreciated.

That aside, I have a couple of nitpicks, but it looks good to me - thanks

Quoted 23 lines

Signed-off-by Jun Piao <[email protected]>

---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

net/9p/trans_virtio.c +1 -3

index d422bfc..0fe9c37 100644
@@ -89,10 +89,8 @@ struct virtio_chan {
 	unsigned long p9_max_pages;
 	/* Scatterlist: can be too big for stack. */
 	struct scatterlist sg[VIRTQUEUE_NUM];
-
-	int tag_len;
 	/*
-	 * tag name to identify a mount Non-null terminated
+	 * tag name to identify a mount null terminated
 	 */
 	char *tag;

net/9p/trans_virtio.c static ssize_t p9_mount_tag_show(struct device *dev, · line 532

Quoted 9 lines

net/9p/trans_virtio.c +2 -3

@@ -529,10 +527,9 @@ static ssize_t p9_mount_tag_show(struct device *dev,
 	vdev = dev_to_virtio(dev);
 	chan = vdev->priv;
 
-	memcpy(buf, chan->tag, chan->tag_len);
-	buf[chan->tag_len] = 0;
+	memcpy(buf, chan->tag, strlen(chan->tag) + 1);
 
-	return chan->tag_len + 1;
+	return strlen(chan->tag) + 1;

Use a local variable for strlen(chan->tag)?

Quoted 21 lines

net/9p/trans_virtio.c +1 -2

 }
 
 static DEVICE_ATTR(mount_tag, 0444, p9_mount_tag_show, NULL);

@@ -585,7 +582,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kmalloc(tag_len, GFP_KERNEL);
+	tag = kzalloc(tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;

@@ -594,7 +591,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
 			   tag, tag_len);
 	chan->tag = tag;
-	chan->tag_len = tag_len;
 	err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
 	if (err) {
 		goto out_free_tag;

net/9p/trans_virtio.c static int p9_virtio_probe(struct virtio_device *vdev) · line 653

Quoted 6 lines

net/9p/trans_virtio.c +1 -2

@@ -654,8 +650,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
 	mutex_lock(&virtio_9p_lock);
 	list_for_each_entry(chan, &virtio_chan_list, chan_list) {
-		if (!strncmp(devname, chan->tag, chan->tag_len) &&
-		    strlen(devname) == chan->tag_len) {
+		if (!strncmp(devname, chan->tag, strlen(chan->tag) + 1)) {

strncmp(x, y, strlen(y)+1) is precisely what strcmp does so let's use the simpler version

Signature · 1 line

Dominique

piaojun

author

2018-08-02 02:01 UTC #2 lore raw

Hi Dominique,

Quoted 36 lines · Dominique Martinet

piaojun wrote on Thu, Aug 02, 2018:
Quoted 4 lines
chan->tag is Non-null terminated which will result in printing messy code when debugging code. So we should add '\0' for tag to make the code more convenient and robust. In addition, I drop char->tag_len to simplify the code.
Some new lines in commit message would be appreciated.
That aside, I have a couple of nitpicks, but it looks good to me - thanks
Quoted 26 lines
Signed-off-by Jun Piao <[email protected]>
---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
net/9p/trans_virtio.c +1 -3
index d422bfc..0fe9c37 100644
@@ -89,10 +89,8 @@ struct virtio_chan {
 	unsigned long p9_max_pages;
 	/* Scatterlist: can be too big for stack. */
 	struct scatterlist sg[VIRTQUEUE_NUM];
-
-	int tag_len;
 	/*
-	 * tag name to identify a mount Non-null terminated
+	 * tag name to identify a mount null terminated
 	 */
 	char *tag;
 
@@ -529,10 +527,9 @@ static ssize_t p9_mount_tag_show(struct device *dev,
 	vdev = dev_to_virtio(dev);
 	chan = vdev->priv;
 

Quoted 9 lines

net/9p/trans_virtio.c static ssize_t p9_mount_tag_show(struct device *dev, · line 532
Quoted 6 lines
net/9p/trans_virtio.c +2 -3
-	memcpy(buf, chan->tag, chan->tag_len);
-	buf[chan->tag_len] = 0;
+	memcpy(buf, chan->tag, strlen(chan->tag) + 1);
 
-	return chan->tag_len + 1;
+	return strlen(chan->tag) + 1;
Use a local variable for strlen(chan->tag)?

Yes, local variable looks better.

Quoted 22 lines

net/9p/trans_virtio.c +1 -2

 }
 
 static DEVICE_ATTR(mount_tag, 0444, p9_mount_tag_show, NULL);

@@ -585,7 +582,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kmalloc(tag_len, GFP_KERNEL);
+	tag = kzalloc(tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;

@@ -594,7 +591,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
 			   tag, tag_len);
 	chan->tag = tag;
-	chan->tag_len = tag_len;
 	err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
 	if (err) {
 		goto out_free_tag;

@@ -654,8 +650,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)

Quoted 9 lines

net/9p/trans_virtio.c static int p9_virtio_probe(struct virtio_device *vdev) · line 653
Quoted 5 lines
net/9p/trans_virtio.c +1 -2
 	mutex_lock(&virtio_9p_lock);
 	list_for_each_entry(chan, &virtio_chan_list, chan_list) {
-		if (!strncmp(devname, chan->tag, chan->tag_len) &&
-		    strlen(devname) == chan->tag_len) {
+		if (!strncmp(devname, chan->tag, strlen(chan->tag) + 1)) {
strncmp(x, y, strlen(y)+1) is precisely what strcmp does so let's use the simpler version

strcmp looks simpler, and I will wait for a while to hear more suggestions, and then post another patch for these fixes.

Thanks,
Jun

Greg Kurz

2018-08-02 13:40 UTC #3 lore raw

Quoted 81 lines · On Thu, 2 Aug 2018 09:59:38 +0800 piaojun <[email protected]> wrote:

Hi Dominique,
Quoted 36 lines · Dominique Martinet
piaojun wrote on Thu, Aug 02, 2018:
Quoted 4 lines
chan->tag is Non-null terminated which will result in printing messy code when debugging code. So we should add '\0' for tag to make the code more convenient and robust. In addition, I drop char->tag_len to simplify the code.
Some new lines in commit message would be appreciated.
That aside, I have a couple of nitpicks, but it looks good to me - thanks
Quoted 26 lines
Signed-off-by Jun Piao <[email protected]>
---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
net/9p/trans_virtio.c +1 -3
index d422bfc..0fe9c37 100644
@@ -89,10 +89,8 @@ struct virtio_chan {
 	unsigned long p9_max_pages;
 	/* Scatterlist: can be too big for stack. */
 	struct scatterlist sg[VIRTQUEUE_NUM];
-
-	int tag_len;
 	/*
-	 * tag name to identify a mount Non-null terminated
+	 * tag name to identify a mount null terminated
 	 */
 	char *tag;
 
@@ -529,10 +527,9 @@ static ssize_t p9_mount_tag_show(struct device *dev,
 	vdev = dev_to_virtio(dev);
 	chan = vdev->priv;
 
Quoted 9 lines
net/9p/trans_virtio.c static ssize_t p9_mount_tag_show(struct device *dev, · line 532
Quoted 6 lines
net/9p/trans_virtio.c +2 -3
-	memcpy(buf, chan->tag, chan->tag_len);
-	buf[chan->tag_len] = 0;
+	memcpy(buf, chan->tag, strlen(chan->tag) + 1);
 
-	return chan->tag_len + 1;
+	return strlen(chan->tag) + 1;  
Use a local variable for strlen(chan->tag)?
Yes, local variable looks better.
Quoted 22 lines
Quoted 22 lines
net/9p/trans_virtio.c +1 -2
 }
 
 static DEVICE_ATTR(mount_tag, 0444, p9_mount_tag_show, NULL);
@@ -585,7 +582,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kmalloc(tag_len, GFP_KERNEL);
+	tag = kzalloc(tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;
@@ -594,7 +591,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
 			   tag, tag_len);
 	chan->tag = tag;
-	chan->tag_len = tag_len;
 	err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
 	if (err) {
 		goto out_free_tag;
@@ -654,8 +650,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
Quoted 9 lines
net/9p/trans_virtio.c static int p9_virtio_probe(struct virtio_device *vdev) · line 653
Quoted 5 lines
net/9p/trans_virtio.c +1 -2
 	mutex_lock(&virtio_9p_lock);
 	list_for_each_entry(chan, &virtio_chan_list, chan_list) {
-		if (!strncmp(devname, chan->tag, chan->tag_len) &&
-		    strlen(devname) == chan->tag_len) {
+		if (!strncmp(devname, chan->tag, strlen(chan->tag) + 1)) {  
strncmp(x, y, strlen(y)+1) is precisely what strcmp does so let's use the simpler version

Quoted 3 lines

strcmp looks simpler, and I will wait for a while to hear more suggestions, and then post another patch for these fixes.

Nothing more to add. Please go ahead.

Quoted 10 lines

Thanks,
Jun
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
V9fs-developer mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/v9fs-developer

piaojun

author

2018-08-03 01:46 UTC #4 lore raw

Hi Greg and Dominique,

We'd better reach an agreement about the patch fix. In my opinion, replacing strlen(chan->tag) with a local variable sounds reasonable, and changing strncmp to strcmp may be little beneficial, as strcmp is more dangerours such as buffer-flow. So I'd like to hear your suggestion for the next version of the patch, or this patch is good enough?

Thanks,
Jun

Quoted 103 lines · Greg Kurz

Quoted 81 lines · On Thu, 2 Aug 2018 09:59:38 +0800 piaojun <[email protected]> wrote:
Hi Dominique,
Quoted 36 lines · Dominique Martinet
piaojun wrote on Thu, Aug 02, 2018:
Quoted 4 lines
chan->tag is Non-null terminated which will result in printing messy code when debugging code. So we should add '\0' for tag to make the code more convenient and robust. In addition, I drop char->tag_len to simplify the code.
Some new lines in commit message would be appreciated.
That aside, I have a couple of nitpicks, but it looks good to me - thanks
Quoted 26 lines
Signed-off-by Jun Piao <[email protected]>
---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
net/9p/trans_virtio.c +1 -3
index d422bfc..0fe9c37 100644
@@ -89,10 +89,8 @@ struct virtio_chan {
 	unsigned long p9_max_pages;
 	/* Scatterlist: can be too big for stack. */
 	struct scatterlist sg[VIRTQUEUE_NUM];
-
-	int tag_len;
 	/*
-	 * tag name to identify a mount Non-null terminated
+	 * tag name to identify a mount null terminated
 	 */
 	char *tag;
 
@@ -529,10 +527,9 @@ static ssize_t p9_mount_tag_show(struct device *dev,
 	vdev = dev_to_virtio(dev);
 	chan = vdev->priv;
 
Quoted 9 lines
net/9p/trans_virtio.c static ssize_t p9_mount_tag_show(struct device *dev, · line 532
Quoted 6 lines
net/9p/trans_virtio.c +2 -3
-	memcpy(buf, chan->tag, chan->tag_len);
-	buf[chan->tag_len] = 0;
+	memcpy(buf, chan->tag, strlen(chan->tag) + 1);
 
-	return chan->tag_len + 1;
+	return strlen(chan->tag) + 1;  
Use a local variable for strlen(chan->tag)?
Yes, local variable looks better.
Quoted 22 lines
Quoted 22 lines
net/9p/trans_virtio.c +1 -2
 }
 
 static DEVICE_ATTR(mount_tag, 0444, p9_mount_tag_show, NULL);
@@ -585,7 +582,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 		err = -EINVAL;
 		goto out_free_vq;
 	}
-	tag = kmalloc(tag_len, GFP_KERNEL);
+	tag = kzalloc(tag_len + 1, GFP_KERNEL);
 	if (!tag) {
 		err = -ENOMEM;
 		goto out_free_vq;
@@ -594,7 +591,6 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 	virtio_cread_bytes(vdev, offsetof(struct virtio_9p_config, tag),
 			   tag, tag_len);
 	chan->tag = tag;
-	chan->tag_len = tag_len;
 	err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
 	if (err) {
 		goto out_free_tag;
@@ -654,8 +650,7 @@ static int p9_virtio_probe(struct virtio_device *vdev)
 
Quoted 9 lines
net/9p/trans_virtio.c static int p9_virtio_probe(struct virtio_device *vdev) · line 653
Quoted 5 lines
net/9p/trans_virtio.c +1 -2
 	mutex_lock(&virtio_9p_lock);
 	list_for_each_entry(chan, &virtio_chan_list, chan_list) {
-		if (!strncmp(devname, chan->tag, chan->tag_len) &&
-		    strlen(devname) == chan->tag_len) {
+		if (!strncmp(devname, chan->tag, strlen(chan->tag) + 1)) {  
strncmp(x, y, strlen(y)+1) is precisely what strcmp does so let's use the simpler version
Quoted 3 lines
strcmp looks simpler, and I will wait for a while to hear more suggestions, and then post another patch for these fixes.
Nothing more to add. Please go ahead.
Quoted 10 lines
Thanks,
Jun
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
V9fs-developer mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/v9fs-developer
.

Dominique Martinet

maintainer

2018-08-03 02:06 UTC #5 lore raw

piaojun wrote on Fri, Aug 03, 2018:

Quoted 1 line

We'd better reach an agreement about the patch fix.

The way I read Greg's comment was that he agreed to the proposed changes and is waiting for a new version.

I'm writing a longer reply than I should because I don't like people saying strncmp is safe just because it's strncmp, feel free to skim through the rest as it's just ranting.

Quoted 2 lines

In my opinion, replacing strlen(chan->tag) with a local variable sounds reasonable,

So we agree here

Quoted 2 lines

and changing strncmp to strcmp may be little beneficial, as strcmp is more dangerours such as buffer-flow.

strcmp is more dangerous for buffer-overflow if you're comparing "unsafe" non-null terminated strings.
This isn't the case here as you've constructed chan->tag yourself and you rely on it being null-terminated by calling strlen() on it yourself.

strncmp(x, y, strlen(y)+1) is at best awkward, but it's a false sense of security if you think this is any better than strcmp here. It implies that:
- y is null-terminated (for strlen() to work)
- x is either null-terminated or longer than y

Here, x is the devname argument to p9_virtio_create, which comes straight from the mount syscall with "copy_mount_string", using strndup_user, which returns a null-terminated string or an error.

(the code is currently not safe if it returns an error, I'm sending another mail about it right after this one as we already have a partial fix)

strcmp(x, y) on the other hand assumes that x and y are null-terminated in this case, which is the same assumptions you have, so is strictly as "safe" as strncmp used that way.
(it could also assume that one is null terminated and the other one is at least as long as that, e.g. when comparing a "char buf[42]" to the constant "foo" then we don't care if buf is null-terminated)

Thanks,

Signature · 1 line

Dominique

Dominique Martinet

maintainer

2018-08-03 02:10 UTC #6 lore raw

Dominique Martinet wrote on Fri, Aug 03, 2018:

Quoted 3 lines

(the code is currently not safe if it returns an error, I'm sending another mail about it right after this one as we already have a partial fix)

I take that one back, ksys_mount() does check for error properly so just the null checks we have in Tomas' patch[1] are enough ; sorry for the double-mail

[1] http://lkml.kernel.org/r/[email protected]

Signature · 1 line

Dominique