* [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag @ 2018-08-02 1:48 piaojun 2018-08-02 1:54 ` Dominique Martinet 0 siblings, 1 reply; 7+ messages in thread From: piaojun @ 2018-08-02 1:48 UTC (permalink / raw) To: Dominique Martinet Cc: akpm, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, Greg Kurz, Linux Kernel Mailing List, v9fs-developer 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; -- ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-02 1:48 [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag piaojun @ 2018-08-02 1:54 ` Dominique Martinet 2018-08-02 1:59 ` piaojun 0 siblings, 1 reply; 7+ messages in thread From: Dominique Martinet @ 2018-08-02 1:54 UTC (permalink / raw) To: piaojun Cc: akpm, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, Greg Kurz, Linux Kernel Mailing List, v9fs-developer 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-02 1:54 ` Dominique Martinet @ 2018-08-02 1:59 ` piaojun 2018-08-02 13:23 ` [V9fs-developer] " Greg Kurz 0 siblings, 1 reply; 7+ messages in thread From: piaojun @ 2018-08-02 1:59 UTC (permalink / raw) To: Dominique Martinet Cc: akpm, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov, Greg Kurz, Linux Kernel Mailing List, v9fs-developer 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-02 1:59 ` piaojun @ 2018-08-02 13:23 ` Greg Kurz 2018-08-03 1:45 ` piaojun 0 siblings, 1 reply; 7+ messages in thread From: Greg Kurz @ 2018-08-02 13:23 UTC (permalink / raw) To: piaojun Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen, Linux Kernel Mailing List, v9fs-developer, Ron Minnich 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-02 13:23 ` [V9fs-developer] " Greg Kurz @ 2018-08-03 1:45 ` piaojun 2018-08-03 2:06 ` Dominique Martinet 0 siblings, 1 reply; 7+ messages in thread From: piaojun @ 2018-08-03 1:45 UTC (permalink / raw) To: Greg Kurz Cc: Dominique Martinet, Latchesar Ionkov, Eric Van Hensbergen, Linux Kernel Mailing List, v9fs-developer, Ron Minnich 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 > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-03 1:45 ` piaojun @ 2018-08-03 2:06 ` Dominique Martinet 2018-08-03 2:09 ` Dominique Martinet 0 siblings, 1 reply; 7+ messages in thread From: Dominique Martinet @ 2018-08-03 2:06 UTC (permalink / raw) To: piaojun Cc: Greg Kurz, Latchesar Ionkov, Eric Van Hensbergen, Linux Kernel Mailing List, v9fs-developer, Ron Minnich 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [V9fs-developer] [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag 2018-08-03 2:06 ` Dominique Martinet @ 2018-08-03 2:09 ` Dominique Martinet 0 siblings, 0 replies; 7+ messages in thread From: Dominique Martinet @ 2018-08-03 2:09 UTC (permalink / raw) To: piaojun; +Cc: Linux Kernel Mailing List, v9fs-developer 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-03 2:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-02 1:48 [PATCH v2] net/9p/trans_virtio.c: add null terminal for mount tag piaojun 2018-08-02 1:54 ` Dominique Martinet 2018-08-02 1:59 ` piaojun 2018-08-02 13:23 ` [V9fs-developer] " Greg Kurz 2018-08-03 1:45 ` piaojun 2018-08-03 2:06 ` Dominique Martinet 2018-08-03 2:09 ` Dominique Martinet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).