Re: [Qemu-devel] [PATCH] 9pfs: fix potential segfault during walk

[Greg Kurz]

On Fri, 16 Sep 2016 09:37:48 +0200
Cédric Le Goater <address@hidden> wrote:

> On 09/16/2016 09:19 AM, Greg Kurz wrote:
> > On Fri, 16 Sep 2016 01:05:11 +0200
> > Greg Kurz <address@hidden> 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 <address@hidden>
> >> ---
> >>
> >> 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.
> 
> 
> 
>