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

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

[piaojun]

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 <piaojun@huawei.com>
---
 net/9p/trans_virtio.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index d422bfc..0fe9c37 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -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;
-- 

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

[Dominique Martinet]

piaojun wrote on Thu, Aug 02, 2018:
> 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

> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>  net/9p/trans_virtio.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index d422bfc..0fe9c37 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -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;

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

>  }
> 
>  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)) {

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

-- 
Dominique

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

[piaojun]

Hi Dominique,

On 2018/8/2 9:54, Dominique Martinet wrote:
> piaojun wrote on Thu, Aug 02, 2018:
>> 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
> 
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>  net/9p/trans_virtio.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index d422bfc..0fe9c37 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -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;
> 
> Use a local variable for strlen(chan->tag)?
> 
Yes, local variable looks better.

>>  }
>>
>>  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)) {
> 
> 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

Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

[Greg Kurz]

On Thu, 2 Aug 2018 09:59:38 +0800
piaojun <piaojun@huawei.com> wrote:

> Hi Dominique,
> 
> On 2018/8/2 9:54, Dominique Martinet wrote:
> > piaojun wrote on Thu, Aug 02, 2018:  
> >> 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
> >   
> >>
> >> Signed-off-by: Jun Piao <piaojun@huawei.com>
> >> ---
> >>  net/9p/trans_virtio.c | 15 +++++----------
> >>  1 file changed, 5 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> >> index d422bfc..0fe9c37 100644
> >> --- a/net/9p/trans_virtio.c
> >> +++ b/net/9p/trans_virtio.c
> >> @@ -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;  
> > 
> > Use a local variable for strlen(chan->tag)?
> >   
> Yes, local variable looks better.
> 
> >>  }
> >>
> >>  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)) {  
> > 
> > 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.
> 

Nothing more to add. Please go ahead.

> 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
> V9fs-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/v9fs-developer

Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

[piaojun]

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

On 2018/8/2 21:23, Greg Kurz wrote:
> On Thu, 2 Aug 2018 09:59:38 +0800
> piaojun <piaojun@huawei.com> wrote:
> 
>> Hi Dominique,
>>
>> On 2018/8/2 9:54, Dominique Martinet wrote:
>>> piaojun wrote on Thu, Aug 02, 2018:  
>>>> 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
>>>   
>>>>
>>>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>>>> ---
>>>>  net/9p/trans_virtio.c | 15 +++++----------
>>>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>>>> index d422bfc..0fe9c37 100644
>>>> --- a/net/9p/trans_virtio.c
>>>> +++ b/net/9p/trans_virtio.c
>>>> @@ -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;  
>>>
>>> Use a local variable for strlen(chan->tag)?
>>>   
>> Yes, local variable looks better.
>>
>>>>  }
>>>>
>>>>  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)) {  
>>>
>>> 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.
>>
> 
> Nothing more to add. Please go ahead.
> 
>> 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
>> V9fs-developer@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/v9fs-developer
> 
> .
> 

Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

[Dominique Martinet]

piaojun wrote on Fri, Aug 03, 2018:
> 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.


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

So we agree here

> 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,
-- 
Dominique

Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag

[Dominique Martinet]

Dominique Martinet wrote on Fri, Aug 03, 2018:
> (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/20180727110558.5479-1-tomasbortoli@gmail.com

-- 
Dominique