* [PATCH 3/7] kvmtool: fix strcpy vulnerabilities
@ 2016-10-18 16:02 G. Campana
2016-11-08 2:08 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: G. Campana @ 2016-10-18 16:02 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, G. Campana
---
virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 17 deletions(-)
diff --git a/virtio/9p.c b/virtio/9p.c
index 9695540..cd93d06 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
{
struct rb_node *node = dev->fids.rb_node;
struct p9_fid *pfid = NULL;
+ size_t len;
while (node) {
struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
@@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
if (!pfid)
return NULL;
+ len = strlen(dev->root_dir);
+ if (len >= sizeof(pfid->path)) {
+ free(pfid);
+ return NULL;
+ }
+
pfid->fid = fid;
- strcpy(pfid->abs_path, dev->root_dir);
- pfid->path = pfid->abs_path + strlen(dev->root_dir);
+ strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
+ pfid->path = pfid->abs_path + strlen(pfid->abs_path);
insert_new_fid(dev, pfid);
@@ -386,6 +393,19 @@ err_out:
return;
}
+static int join_path(struct p9_fid *fid, const char *name)
+{
+ size_t len, size;
+
+ size = sizeof(fid->abs_path) - (fid->path - fid->abs_path);
+ len = strlen(name);
+ if (len >= size)
+ return -1;
+
+ strncpy(fid->path, name, size);
+ return 0;
+}
+
static void virtio_p9_walk(struct p9_dev *p9dev,
struct p9_pdu *pdu, u32 *outlen)
{
@@ -404,7 +424,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
if (nwname) {
struct p9_fid *fid = get_fid(p9dev, fid_val);
- strcpy(new_fid->path, fid->path);
+ join_path(new_fid, fid->path);
/* skip the space for count */
pdu->write_offset += sizeof(u16);
for (i = 0; i < nwname; i++) {
@@ -424,7 +444,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
goto err_out;
stat2qid(&st, &wqid);
- strcpy(new_fid->path, tmp);
+ join_path(new_fid, tmp);
new_fid->uid = fid->uid;
nwqid++;
virtio_p9_pdu_writef(pdu, "Q", &wqid);
@@ -435,7 +455,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
*/
pdu->write_offset += sizeof(u16);
old_fid = get_fid(p9dev, fid_val);
- strcpy(new_fid->path, old_fid->path);
+ join_path(new_fid, old_fid->path);
new_fid->uid = old_fid->uid;
}
*outlen = pdu->write_offset;
@@ -471,7 +491,7 @@ static void virtio_p9_attach(struct p9_dev *p9dev,
fid = get_fid(p9dev, fid_val);
fid->uid = uid;
- strcpy(fid->path, "/");
+ join_path(fid, "/");
virtio_p9_pdu_writef(pdu, "Q", &qid);
*outlen = pdu->write_offset;
@@ -1115,20 +1135,24 @@ static int virtio_p9_ancestor(char *path, char *ancestor)
return 0;
}
-static void virtio_p9_fix_path(char *fid_path, char *old_name, char *new_name)
+static int virtio_p9_fix_path(struct p9_fid *fid, char *old_name, char *new_name)
{
- char tmp_name[PATH_MAX];
+ int ret;
+ char *p, tmp_name[PATH_MAX];
size_t rp_sz = strlen(old_name);
- if (rp_sz == strlen(fid_path)) {
+ if (rp_sz == strlen(fid->path)) {
/* replace the full name */
- strcpy(fid_path, new_name);
- return;
+ p = new_name;
+ } else {
+ /* save the trailing path details */
+ ret = snprintf(tmp_name, sizeof(tmp_name), "%s%s", new_name, fid->path + rp_sz);
+ if (ret >= (int)sizeof(tmp_name))
+ return -1;
+ p = tmp_name;
}
- /* save the trailing path details */
- strcpy(tmp_name, fid_path + rp_sz);
- sprintf(fid_path, "%s%s", new_name, tmp_name);
- return;
+
+ return join_path(fid, p);
}
static void rename_fids(struct p9_dev *p9dev, char *old_name, char *new_name)
@@ -1139,7 +1163,7 @@ static void rename_fids(struct p9_dev *p9dev, char *old_name, char *new_name)
struct p9_fid *fid = rb_entry(node, struct p9_fid, node);
if (fid->fid != P9_NOFID && virtio_p9_ancestor(fid->path, old_name)) {
- virtio_p9_fix_path(fid->path, old_name, new_name);
+ virtio_p9_fix_path(fid, old_name, new_name);
}
node = rb_next(node);
}
@@ -1539,7 +1563,9 @@ int virtio_9p__register(struct kvm *kvm, const char *root, const char *tag_name)
goto free_p9dev;
}
- strcpy(p9dev->root_dir, root);
+ strncpy(p9dev->root_dir, root, sizeof(p9dev->root_dir));
+ p9dev->root_dir[sizeof(p9dev->root_dir)-1] = '\x00';
+
p9dev->config->tag_len = strlen(tag_name);
if (p9dev->config->tag_len > MAX_TAG_LEN) {
err = -EINVAL;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/7] kvmtool: fix strcpy vulnerabilities
2016-10-18 16:02 [PATCH 3/7] kvmtool: fix strcpy vulnerabilities G. Campana
@ 2016-11-08 2:08 ` Will Deacon
2016-11-10 15:15 ` G. Campana
0 siblings, 1 reply; 5+ messages in thread
From: Will Deacon @ 2016-11-08 2:08 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
On Tue, Oct 18, 2016 at 06:02:52PM +0200, G. Campana wrote:
> ---
> virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 17 deletions(-)
>
> diff --git a/virtio/9p.c b/virtio/9p.c
> index 9695540..cd93d06 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
> {
> struct rb_node *node = dev->fids.rb_node;
> struct p9_fid *pfid = NULL;
> + size_t len;
>
> while (node) {
> struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
> @@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
> if (!pfid)
> return NULL;
>
> + len = strlen(dev->root_dir);
> + if (len >= sizeof(pfid->path)) {
> + free(pfid);
> + return NULL;
> + }
This doesn't make sense to me -- pfid->path is just a NULL ptr at this
stage. Did you mean abs_path?
> +
> pfid->fid = fid;
> - strcpy(pfid->abs_path, dev->root_dir);
> - pfid->path = pfid->abs_path + strlen(dev->root_dir);
> + strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
But if you did mean abs_path, why use strncpy here?
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/7] kvmtool: fix strcpy vulnerabilities
2016-11-08 2:08 ` Will Deacon
@ 2016-11-10 15:15 ` G. Campana
2016-11-17 15:30 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: G. Campana @ 2016-11-10 15:15 UTC (permalink / raw)
To: Will Deacon; +Cc: kvm, andre.przywara
On 08/11/2016 03:08, Will Deacon wrote:
> On Tue, Oct 18, 2016 at 06:02:52PM +0200, G. Campana wrote:
>> ---
>> virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 43 insertions(+), 17 deletions(-)
>>
>> diff --git a/virtio/9p.c b/virtio/9p.c
>> index 9695540..cd93d06 100644
>> --- a/virtio/9p.c
>> +++ b/virtio/9p.c
>> @@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
>> {
>> struct rb_node *node = dev->fids.rb_node;
>> struct p9_fid *pfid = NULL;
>> + size_t len;
>>
>> while (node) {
>> struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
>> @@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
>> if (!pfid)
>> return NULL;
>>
>> + len = strlen(dev->root_dir);
>> + if (len >= sizeof(pfid->path)) {
>> + free(pfid);
>> + return NULL;
>> + }
>
> This doesn't make sense to me -- pfid->path is just a NULL ptr at this
> stage. Did you mean abs_path?
>
Good catch. Indeed, I meant abs_path.
>> +
>> pfid->fid = fid;
>> - strcpy(pfid->abs_path, dev->root_dir);
>> - pfid->path = pfid->abs_path + strlen(dev->root_dir);
>> + strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
>
> But if you did mean abs_path, why use strncpy here?
>
It ensures that the string is null terminated.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/7] kvmtool: fix strcpy vulnerabilities
2016-11-10 15:15 ` G. Campana
@ 2016-11-17 15:30 ` Will Deacon
2016-11-18 15:33 ` G. Campana
0 siblings, 1 reply; 5+ messages in thread
From: Will Deacon @ 2016-11-17 15:30 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
On Thu, Nov 10, 2016 at 04:15:12PM +0100, G. Campana wrote:
> On 08/11/2016 03:08, Will Deacon wrote:
> > On Tue, Oct 18, 2016 at 06:02:52PM +0200, G. Campana wrote:
> >> ---
> >> virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
> >> 1 file changed, 43 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/virtio/9p.c b/virtio/9p.c
> >> index 9695540..cd93d06 100644
> >> --- a/virtio/9p.c
> >> +++ b/virtio/9p.c
> >> @@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
> >> {
> >> struct rb_node *node = dev->fids.rb_node;
> >> struct p9_fid *pfid = NULL;
> >> + size_t len;
> >>
> >> while (node) {
> >> struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
> >> @@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
> >> if (!pfid)
> >> return NULL;
> >>
> >> + len = strlen(dev->root_dir);
> >> + if (len >= sizeof(pfid->path)) {
> >> + free(pfid);
> >> + return NULL;
> >> + }
> >
> > This doesn't make sense to me -- pfid->path is just a NULL ptr at this
> > stage. Did you mean abs_path?
> >
> Good catch. Indeed, I meant abs_path.
>
> >> +
> >> pfid->fid = fid;
> >> - strcpy(pfid->abs_path, dev->root_dir);
> >> - pfid->path = pfid->abs_path + strlen(dev->root_dir);
> >> + strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
> >
> > But if you did mean abs_path, why use strncpy here?
> >
> It ensures that the string is null terminated.
Given that we already did a strlen on root_dir, and we know that abs_path
is big enough to hold it, I still think this isn't needed.
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3/7] kvmtool: fix strcpy vulnerabilities
2016-11-17 15:30 ` Will Deacon
@ 2016-11-18 15:33 ` G. Campana
0 siblings, 0 replies; 5+ messages in thread
From: G. Campana @ 2016-11-18 15:33 UTC (permalink / raw)
To: Will Deacon, G. Campana; +Cc: kvm, andre.przywara
On 11/17/2016 04:30 PM, Will Deacon wrote:
> On Thu, Nov 10, 2016 at 04:15:12PM +0100, G. Campana wrote:
>> On 08/11/2016 03:08, Will Deacon wrote:
>>> On Tue, Oct 18, 2016 at 06:02:52PM +0200, G. Campana wrote:
>>>> ---
>>>> virtio/9p.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----------------
>>>> 1 file changed, 43 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/virtio/9p.c b/virtio/9p.c
>>>> index 9695540..cd93d06 100644
>>>> --- a/virtio/9p.c
>>>> +++ b/virtio/9p.c
>>>> @@ -28,6 +28,7 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
>>>> {
>>>> struct rb_node *node = dev->fids.rb_node;
>>>> struct p9_fid *pfid = NULL;
>>>> + size_t len;
>>>>
>>>> while (node) {
>>>> struct p9_fid *cur = rb_entry(node, struct p9_fid, node);
>>>> @@ -45,9 +46,15 @@ static struct p9_fid *find_or_create_fid(struct p9_dev *dev, u32 fid)
>>>> if (!pfid)
>>>> return NULL;
>>>>
>>>> + len = strlen(dev->root_dir);
>>>> + if (len >= sizeof(pfid->path)) {
>>>> + free(pfid);
>>>> + return NULL;
>>>> + }
>>>
>>> This doesn't make sense to me -- pfid->path is just a NULL ptr at this
>>> stage. Did you mean abs_path?
>>>
>> Good catch. Indeed, I meant abs_path.
>>
>>>> +
>>>> pfid->fid = fid;
>>>> - strcpy(pfid->abs_path, dev->root_dir);
>>>> - pfid->path = pfid->abs_path + strlen(dev->root_dir);
>>>> + strncpy(pfid->abs_path, dev->root_dir, sizeof(pfid->abs_path));
>>>
>>> But if you did mean abs_path, why use strncpy here?
>>>
>> It ensures that the string is null terminated.
>
> Given that we already did a strlen on root_dir, and we know that abs_path
> is big enough to hold it, I still think this isn't needed.
>
Even if we know that abs_path is big enough, I prefer strncpy over
strcpy because it won't mislead static analysis tools and the overhead
is negligible. Anyway, it doesn't matter in the end, and I can submit a
new patch if you want.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-18 15:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-18 16:02 [PATCH 3/7] kvmtool: fix strcpy vulnerabilities G. Campana
2016-11-08 2:08 ` Will Deacon
2016-11-10 15:15 ` G. Campana
2016-11-17 15:30 ` Will Deacon
2016-11-18 15:33 ` G. Campana
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).