* [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
@ 2016-10-18 16:03 G. Campana
2016-11-08 2:38 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: G. Campana @ 2016-10-18 16:03 UTC (permalink / raw)
To: Will.Deacon; +Cc: kvm, andre.przywara, G. Campana
---
virtio/9p.c | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/virtio/9p.c b/virtio/9p.c
index 5b2d261..3259b79 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -91,18 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid)
return new;
}
-static int rel_to_abs(struct p9_dev *p9dev, const char *path, char *abs_path,
- size_t size)
-{
- int ret;
-
- ret = snprintf(abs_path, size, "%s/%s", p9dev->root_dir, path);
- if (ret >= (int)size)
- return -1;
-
- return 0;
-}
-
static void stat2qid(struct stat *st, struct p9_qid *qid)
{
*qid = (struct p9_qid) {
@@ -266,6 +254,28 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
return 0;
}
+static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st)
+{
+ int ret;
+ char full_path[PATH_MAX];
+
+ ret = snprintf(full_path, sizeof(full_path), "%s/%s", p9dev->root_dir, path);
+ if (ret >= (int)sizeof(full_path)) {
+ errno = ENAMETOOLONG;
+ return -1;
+ }
+
+ if (path_is_illegal(full_path)) {
+ errno = EACCES;
+ return -1;
+ }
+
+ if (lstat(full_path, st) != 0)
+ return -1;
+
+ return 0;
+}
+
static void virtio_p9_open(struct p9_dev *p9dev,
struct p9_pdu *pdu, u32 *outlen)
{
@@ -440,7 +450,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
for (i = 0; i < nwname; i++) {
struct stat st;
char tmp[PATH_MAX] = {0};
- char full_path[PATH_MAX];
char *str;
int ret;
@@ -455,12 +464,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
free(str);
- if (rel_to_abs(p9dev, tmp, full_path, sizeof(full_path)) != 0) {
- errno = ENAMETOOLONG;
- goto err_out;
- }
-
- if (lstat(full_path, &st) < 0)
+ if (stat_rel(p9dev, tmp, &st) != 0)
goto err_out;
stat2qid(&st, &wqid);
@@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
struct stat st;
struct p9_fid *fid;
struct dirent *dent;
- char full_path[PATH_MAX];
u64 offset, old_offset;
rcount = 0;
@@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
break;
}
old_offset = dent->d_off;
- if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
- errno = ENAMETOOLONG;
- goto err_out;
- }
- lstat(full_path, &st);
+ if (stat_rel(p9dev, dent->d_name, &st) != 0)
+ memset(&st, -1, sizeof(st));
stat2qid(&st, &qid);
read = pdu->write_offset;
virtio_p9_pdu_writef(pdu, "Qqbs", &qid, dent->d_off,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
2016-10-18 16:03 [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs() G. Campana
@ 2016-11-08 2:38 ` Will Deacon
2016-11-10 15:18 ` G. Campana
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2016-11-08 2:38 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
> ---
> virtio/9p.c | 50 +++++++++++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/virtio/9p.c b/virtio/9p.c
> index 5b2d261..3259b79 100644
> --- a/virtio/9p.c
> +++ b/virtio/9p.c
> @@ -91,18 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid)
> return new;
> }
>
> -static int rel_to_abs(struct p9_dev *p9dev, const char *path, char *abs_path,
> - size_t size)
> -{
> - int ret;
> -
> - ret = snprintf(abs_path, size, "%s/%s", p9dev->root_dir, path);
> - if (ret >= (int)size)
> - return -1;
> -
> - return 0;
> -}
Can this be merged with patch 5, where you introduced rel_to_abs?
> static void stat2qid(struct stat *st, struct p9_qid *qid)
> {
> *qid = (struct p9_qid) {
> @@ -266,6 +254,28 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
> return 0;
> }
>
> +static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st)
> +{
> + int ret;
> + char full_path[PATH_MAX];
> +
> + ret = snprintf(full_path, sizeof(full_path), "%s/%s", p9dev->root_dir, path);
> + if (ret >= (int)sizeof(full_path)) {
> + errno = ENAMETOOLONG;
> + return -1;
> + }
> +
> + if (path_is_illegal(full_path)) {
> + errno = EACCES;
> + return -1;
> + }
Up to this point, you've just reimplemented most of get_full_path. Is it
worth giving these two functions a comment "concatenate these two path
components and check if the result is legal" backend?
> + if (lstat(full_path, st) != 0)
> + return -1;
> +
> + return 0;
> +}
> +
> static void virtio_p9_open(struct p9_dev *p9dev,
> struct p9_pdu *pdu, u32 *outlen)
> {
> @@ -440,7 +450,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
> for (i = 0; i < nwname; i++) {
> struct stat st;
> char tmp[PATH_MAX] = {0};
> - char full_path[PATH_MAX];
> char *str;
> int ret;
>
> @@ -455,12 +464,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>
> free(str);
>
> - if (rel_to_abs(p9dev, tmp, full_path, sizeof(full_path)) != 0) {
> - errno = ENAMETOOLONG;
> - goto err_out;
> - }
> -
> - if (lstat(full_path, &st) < 0)
> + if (stat_rel(p9dev, tmp, &st) != 0)
> goto err_out;
>
> stat2qid(&st, &wqid);
> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> struct stat st;
> struct p9_fid *fid;
> struct dirent *dent;
> - char full_path[PATH_MAX];
> u64 offset, old_offset;
>
> rcount = 0;
> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> break;
> }
> old_offset = dent->d_off;
> - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
> - errno = ENAMETOOLONG;
> - goto err_out;
> - }
> - lstat(full_path, &st);
> + if (stat_rel(p9dev, dent->d_name, &st) != 0)
> + memset(&st, -1, sizeof(st));
Why the memset, and not goto err_out?
Will
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
2016-11-08 2:38 ` Will Deacon
@ 2016-11-10 15:18 ` G. Campana
2016-11-17 12:20 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: G. Campana @ 2016-11-10 15:18 UTC (permalink / raw)
To: Will Deacon; +Cc: kvm, andre.przywara
On 08/11/2016 03:38, Will Deacon wrote:
> On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
>> ---
>> virtio/9p.c | 50 +++++++++++++++++++++++++-------------------------
>> 1 file changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/virtio/9p.c b/virtio/9p.c
>> index 5b2d261..3259b79 100644
>> --- a/virtio/9p.c
>> +++ b/virtio/9p.c
>> @@ -91,18 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid)
>> return new;
>> }
>>
>> -static int rel_to_abs(struct p9_dev *p9dev, const char *path, char *abs_path,
>> - size_t size)
>> -{
>> - int ret;
>> -
>> - ret = snprintf(abs_path, size, "%s/%s", p9dev->root_dir, path);
>> - if (ret >= (int)size)
>> - return -1;
>> -
>> - return 0;
>> -}
>
> Can this be merged with patch 5, where you introduced rel_to_abs?
>
Yes, I reworked this patch.
>> static void stat2qid(struct stat *st, struct p9_qid *qid)
>> {
>> *qid = (struct p9_qid) {
>> @@ -266,6 +254,28 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid,
>> return 0;
>> }
>>
>> +static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st)
>> +{
>> + int ret;
>> + char full_path[PATH_MAX];
>> +
>> + ret = snprintf(full_path, sizeof(full_path), "%s/%s", p9dev->root_dir, path);
>> + if (ret >= (int)sizeof(full_path)) {
>> + errno = ENAMETOOLONG;
>> + return -1;
>> + }
>> +
>> + if (path_is_illegal(full_path)) {
>> + errno = EACCES;
>> + return -1;
>> + }
>
> Up to this point, you've just reimplemented most of get_full_path. Is it
> worth giving these two functions a comment "concatenate these two path
> components and check if the result is legal" backend?
>
I introduced get_full_path_helper(), which is called by stat_rel() and
get_full_path().
>> + if (lstat(full_path, st) != 0)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> static void virtio_p9_open(struct p9_dev *p9dev,
>> struct p9_pdu *pdu, u32 *outlen)
>> {
>> @@ -440,7 +450,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>> for (i = 0; i < nwname; i++) {
>> struct stat st;
>> char tmp[PATH_MAX] = {0};
>> - char full_path[PATH_MAX];
>> char *str;
>> int ret;
>>
>> @@ -455,12 +464,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev,
>>
>> free(str);
>>
>> - if (rel_to_abs(p9dev, tmp, full_path, sizeof(full_path)) != 0) {
>> - errno = ENAMETOOLONG;
>> - goto err_out;
>> - }
>> -
>> - if (lstat(full_path, &st) < 0)
>> + if (stat_rel(p9dev, tmp, &st) != 0)
>> goto err_out;
>>
>> stat2qid(&st, &wqid);
>> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>> struct stat st;
>> struct p9_fid *fid;
>> struct dirent *dent;
>> - char full_path[PATH_MAX];
>> u64 offset, old_offset;
>>
>> rcount = 0;
>> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>> break;
>> }
>> old_offset = dent->d_off;
>> - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
>> - errno = ENAMETOOLONG;
>> - goto err_out;
>> - }
>> - lstat(full_path, &st);
>> + if (stat_rel(p9dev, dent->d_name, &st) != 0)
>> + memset(&st, -1, sizeof(st));
>
> Why the memset, and not goto err_out?
>
Because the user may not be allowed to stat some entries in a directory
and it shouldn't make readdir() fail.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
2016-11-10 15:18 ` G. Campana
@ 2016-11-17 12:20 ` Will Deacon
2016-11-18 15:33 ` G. Campana
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2016-11-17 12:20 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
On Thu, Nov 10, 2016 at 04:18:54PM +0100, G. Campana wrote:
> On 08/11/2016 03:38, Will Deacon wrote:
> > On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
> >> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> >> struct stat st;
> >> struct p9_fid *fid;
> >> struct dirent *dent;
> >> - char full_path[PATH_MAX];
> >> u64 offset, old_offset;
> >>
> >> rcount = 0;
> >> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> >> break;
> >> }
> >> old_offset = dent->d_off;
> >> - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
> >> - errno = ENAMETOOLONG;
> >> - goto err_out;
> >> - }
> >> - lstat(full_path, &st);
> >> + if (stat_rel(p9dev, dent->d_name, &st) != 0)
> >> + memset(&st, -1, sizeof(st));
> >
> > Why the memset, and not goto err_out?
> >
> Because the user may not be allowed to stat some entries in a directory
> and it shouldn't make readdir() fail.
Ok, but is memsetting to -1 really the right thing to do? This gets
"converted" into a p9_qid_t, which will then look pretty strange (path
and version will be set to 0xff, type will be set to P9_QTDIR).
Does 9p not have a better way to communicate that the stat failed?
Will
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
2016-11-17 12:20 ` Will Deacon
@ 2016-11-18 15:33 ` G. Campana
2016-11-18 15:37 ` Will Deacon
0 siblings, 1 reply; 6+ 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 01:20 PM, Will Deacon wrote:
> On Thu, Nov 10, 2016 at 04:18:54PM +0100, G. Campana wrote:
>> On 08/11/2016 03:38, Will Deacon wrote:
>>> On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
>>>> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>>>> struct stat st;
>>>> struct p9_fid *fid;
>>>> struct dirent *dent;
>>>> - char full_path[PATH_MAX];
>>>> u64 offset, old_offset;
>>>>
>>>> rcount = 0;
>>>> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
>>>> break;
>>>> }
>>>> old_offset = dent->d_off;
>>>> - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
>>>> - errno = ENAMETOOLONG;
>>>> - goto err_out;
>>>> - }
>>>> - lstat(full_path, &st);
>>>> + if (stat_rel(p9dev, dent->d_name, &st) != 0)
>>>> + memset(&st, -1, sizeof(st));
>>>
>>> Why the memset, and not goto err_out?
>>>
>> Because the user may not be allowed to stat some entries in a directory
>> and it shouldn't make readdir() fail.
>
> Ok, but is memsetting to -1 really the right thing to do? This gets
> "converted" into a p9_qid_t, which will then look pretty strange (path
> and version will be set to 0xff, type will be set to P9_QTDIR).
>
Before this patch, st was either uninitialized or invalid if lstat
failed, hence the memset call which doesn't break the logic of this
function. I only tried to fix vulnerabilities in this patch series, and
I think this issue deserves a separate patch. What do you think?
> Does 9p not have a better way to communicate that the stat failed?
>
> Will
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs()
2016-11-18 15:33 ` G. Campana
@ 2016-11-18 15:37 ` Will Deacon
0 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2016-11-18 15:37 UTC (permalink / raw)
To: G. Campana; +Cc: kvm, andre.przywara
On Fri, Nov 18, 2016 at 04:33:07PM +0100, G. Campana wrote:
> On 11/17/2016 01:20 PM, Will Deacon wrote:
> > On Thu, Nov 10, 2016 at 04:18:54PM +0100, G. Campana wrote:
> >> On 08/11/2016 03:38, Will Deacon wrote:
> >>> On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote:
> >>>> @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> >>>> struct stat st;
> >>>> struct p9_fid *fid;
> >>>> struct dirent *dent;
> >>>> - char full_path[PATH_MAX];
> >>>> u64 offset, old_offset;
> >>>>
> >>>> rcount = 0;
> >>>> @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev,
> >>>> break;
> >>>> }
> >>>> old_offset = dent->d_off;
> >>>> - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) {
> >>>> - errno = ENAMETOOLONG;
> >>>> - goto err_out;
> >>>> - }
> >>>> - lstat(full_path, &st);
> >>>> + if (stat_rel(p9dev, dent->d_name, &st) != 0)
> >>>> + memset(&st, -1, sizeof(st));
> >>>
> >>> Why the memset, and not goto err_out?
> >>>
> >> Because the user may not be allowed to stat some entries in a directory
> >> and it shouldn't make readdir() fail.
> >
> > Ok, but is memsetting to -1 really the right thing to do? This gets
> > "converted" into a p9_qid_t, which will then look pretty strange (path
> > and version will be set to 0xff, type will be set to P9_QTDIR).
> >
> Before this patch, st was either uninitialized or invalid if lstat
> failed, hence the memset call which doesn't break the logic of this
> function. I only tried to fix vulnerabilities in this patch series, and
> I think this issue deserves a separate patch. What do you think?
Well, how about just skipping entries where the stat failed and continuing
around the loop. Does that work?
Will
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-18 15:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-18 16:03 [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs() G. Campana
2016-11-08 2:38 ` Will Deacon
2016-11-10 15:18 ` G. Campana
2016-11-17 12:20 ` Will Deacon
2016-11-18 15:33 ` G. Campana
2016-11-18 15:37 ` Will Deacon
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).