| Message ID | 147398054143.25432.18026157491978434193.stgit@bahia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 16 Sep 2016 01:05:11 +0200 Greg Kurz <groug@kaod.org> wrote: > If the call to fid_to_qid() returns an error, we will call v9fs_path_free() > on uninitialized paths. > I'll add this to the changelog: It is a regression introduced by the following commit: 56f101ecce0e 9pfs: handle walk of ".." in the root directory > Let's fix this by initializing dpath and path before calling fid_to_qid(). > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > > Thanks Paolo (and Coverity) for spotting this. > > Cc'ing stable as this is a regression introduced in 2.7. It is also present > in Michael's stable-2.6-staging branch. > > hw/9pfs/9p.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index dfe293d11d1c..91a497079acb 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque) > goto out_nofid; > } > > + v9fs_path_init(&dpath); > + v9fs_path_init(&path); > + > err = fid_to_qid(pdu, fidp, &qid); > if (err < 0) { > goto out; > } > > - v9fs_path_init(&dpath); > - v9fs_path_init(&path); > /* > * Both dpath and path initially poin to fidp. > * Needed to handle request with nwnames == 0 >
On 09/16/2016 09:19 AM, Greg Kurz wrote: > On Fri, 16 Sep 2016 01:05:11 +0200 > Greg Kurz <groug@kaod.org> wrote: > >> If the call to fid_to_qid() returns an error, we will call v9fs_path_free() >> on uninitialized paths. >> > > I'll add this to the changelog: > > It is a regression introduced by the following commit: > > 56f101ecce0e 9pfs: handle walk of ".." in the root directory > >> Let's fix this by initializing dpath and path before calling fid_to_qid(). >> >> Signed-off-by: Greg Kurz <groug@kaod.org> >> --- >> >> Thanks Paolo (and Coverity) for spotting this. >> >> Cc'ing stable as this is a regression introduced in 2.7. It is also present >> in Michael's stable-2.6-staging branch. >> >> hw/9pfs/9p.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >> index dfe293d11d1c..91a497079acb 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque) >> goto out_nofid; >> } >> >> + v9fs_path_init(&dpath); >> + v9fs_path_init(&path); >> + >> err = fid_to_qid(pdu, fidp, &qid); >> if (err < 0) { >> goto out; >> } >> >> - v9fs_path_init(&dpath); >> - v9fs_path_init(&path); >> /* >> * Both dpath and path initially poin to fidp. >> * Needed to handle request with nwnames == 0 >> > > There is still a possible segv I think, in out_nofid : if (nwnames && nwnames <= P9_MAXWELEM) { for (name_idx = 0; name_idx < nwnames; name_idx++) { v9fs_string_free(&wnames[name_idx]); &wnames[name_idx] could NULL if pdu_unmarshal() fails. C.
On Fri, 16 Sep 2016 09:37:48 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 09/16/2016 09:19 AM, Greg Kurz wrote: > > On Fri, 16 Sep 2016 01:05:11 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > >> If the call to fid_to_qid() returns an error, we will call v9fs_path_free() > >> on uninitialized paths. > >> > > > > I'll add this to the changelog: > > > > It is a regression introduced by the following commit: > > > > 56f101ecce0e 9pfs: handle walk of ".." in the root directory > > > >> Let's fix this by initializing dpath and path before calling fid_to_qid(). > >> > >> Signed-off-by: Greg Kurz <groug@kaod.org> > >> --- > >> > >> Thanks Paolo (and Coverity) for spotting this. > >> > >> Cc'ing stable as this is a regression introduced in 2.7. It is also present > >> in Michael's stable-2.6-staging branch. > >> > >> hw/9pfs/9p.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > >> index dfe293d11d1c..91a497079acb 100644 > >> --- a/hw/9pfs/9p.c > >> +++ b/hw/9pfs/9p.c > >> @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque) > >> goto out_nofid; > >> } > >> > >> + v9fs_path_init(&dpath); > >> + v9fs_path_init(&path); > >> + > >> err = fid_to_qid(pdu, fidp, &qid); > >> if (err < 0) { > >> goto out; > >> } > >> > >> - v9fs_path_init(&dpath); > >> - v9fs_path_init(&path); > >> /* > >> * Both dpath and path initially poin to fidp. > >> * Needed to handle request with nwnames == 0 > >> > > > > > > > There is still a possible segv I think, in out_nofid : > > if (nwnames && nwnames <= P9_MAXWELEM) { > for (name_idx = 0; name_idx < nwnames; name_idx++) { > v9fs_string_free(&wnames[name_idx]); > > &wnames[name_idx] could NULL if pdu_unmarshal() fails. > We're safe because wnames[] is fully allocated and zeroed: if (nwnames && nwnames <= P9_MAXWELEM) { wnames = g_malloc0(sizeof(wnames[0]) * nwnames); <--- here qids = g_malloc0(sizeof(qids[0]) * nwnames); for (i = 0; i < nwnames; i++) { err = pdu_unmarshal(pdu, offset, "s", &wnames[i]); if (err < 0) { goto out_nofid; } But I agree that the calls to v9fs_string_free() without the corresponding calls to v9fs_string_init(), as it is done everywhere else, is disturbing and should be addressed. Thanks. -- Greg > > > C. > > > >
On 09/16/2016 10:52 AM, Greg Kurz wrote: > On Fri, 16 Sep 2016 09:37:48 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 09/16/2016 09:19 AM, Greg Kurz wrote: >>> On Fri, 16 Sep 2016 01:05:11 +0200 >>> Greg Kurz <groug@kaod.org> wrote: >>> >>>> If the call to fid_to_qid() returns an error, we will call v9fs_path_free() >>>> on uninitialized paths. >>>> >>> >>> I'll add this to the changelog: >>> >>> It is a regression introduced by the following commit: >>> >>> 56f101ecce0e 9pfs: handle walk of ".." in the root directory >>> >>>> Let's fix this by initializing dpath and path before calling fid_to_qid(). >>>> >>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>> --- >>>> >>>> Thanks Paolo (and Coverity) for spotting this. >>>> >>>> Cc'ing stable as this is a regression introduced in 2.7. It is also present >>>> in Michael's stable-2.6-staging branch. >>>> >>>> hw/9pfs/9p.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >>>> index dfe293d11d1c..91a497079acb 100644 >>>> --- a/hw/9pfs/9p.c >>>> +++ b/hw/9pfs/9p.c >>>> @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque) >>>> goto out_nofid; >>>> } >>>> >>>> + v9fs_path_init(&dpath); >>>> + v9fs_path_init(&path); >>>> + >>>> err = fid_to_qid(pdu, fidp, &qid); >>>> if (err < 0) { >>>> goto out; >>>> } >>>> >>>> - v9fs_path_init(&dpath); >>>> - v9fs_path_init(&path); >>>> /* >>>> * Both dpath and path initially poin to fidp. >>>> * Needed to handle request with nwnames == 0 >>>> >>> >>> >> >> >> There is still a possible segv I think, in out_nofid : >> >> if (nwnames && nwnames <= P9_MAXWELEM) { >> for (name_idx = 0; name_idx < nwnames; name_idx++) { >> v9fs_string_free(&wnames[name_idx]); >> >> &wnames[name_idx] could NULL if pdu_unmarshal() fails. >> > > We're safe because wnames[] is fully allocated and zeroed: ah yes. I got confused by the structures. Reviewed-by: Cédric Le Goater <clg@kaod.org> C. > if (nwnames && nwnames <= P9_MAXWELEM) { > wnames = g_malloc0(sizeof(wnames[0]) * nwnames); <--- here > qids = g_malloc0(sizeof(qids[0]) * nwnames); > for (i = 0; i < nwnames; i++) { > err = pdu_unmarshal(pdu, offset, "s", &wnames[i]); > if (err < 0) { > goto out_nofid; > } > > But I agree that the calls to v9fs_string_free() without the corresponding > calls to v9fs_string_init(), as it is done everywhere else, is disturbing > and should be addressed. > > Thanks. > > -- > Greg > >> >> >> C. >> >> >> >> >
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index dfe293d11d1c..91a497079acb 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1320,13 +1320,14 @@ static void v9fs_walk(void *opaque) goto out_nofid; } + v9fs_path_init(&dpath); + v9fs_path_init(&path); + err = fid_to_qid(pdu, fidp, &qid); if (err < 0) { goto out; } - v9fs_path_init(&dpath); - v9fs_path_init(&path); /* * Both dpath and path initially poin to fidp. * Needed to handle request with nwnames == 0
If the call to fid_to_qid() returns an error, we will call v9fs_path_free() on uninitialized paths. Let's fix this by initializing dpath and path before calling fid_to_qid(). Signed-off-by: Greg Kurz <groug@kaod.org> --- Thanks Paolo (and Coverity) for spotting this. Cc'ing stable as this is a regression introduced in 2.7. It is also present in Michael's stable-2.6-staging branch. hw/9pfs/9p.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)