Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop

[Greg Kurz]

On Thu, 07 May 2020 13:46:50 +0200
Christian Schoenebeck <address@hidden> wrote:

> On Mittwoch, 6. Mai 2020 19:54:15 CEST Greg Kurz wrote:
> > On Wed, 06 May 2020 15:36:16 +0200
> > 
> > Christian Schoenebeck <address@hidden> wrote:
> > > On Mittwoch, 6. Mai 2020 15:05:23 CEST Christian Schoenebeck wrote:
> > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > index 9e046f7acb51..ac84ae804496 100644
> > > > > --- a/hw/9pfs/9p.c
> > > > > +++ b/hw/9pfs/9p.c
> > > > > @@ -2170,7 +2170,7 @@ static int coroutine_fn
> > > > > v9fs_do_readdir_with_stat(V9fsPDU *pdu, int32_t count = 0;
> > > > > 
> > > > >      struct stat stbuf;
> > > > >      off_t saved_dir_pos;
> > > > > 
> > > > > -    struct dirent *dent;
> > > > > +    struct dirent dent;
> > > 
> > > One more: since this dirent structure is now on the stack, it should
> > > better be initialized for safety reasons.
> > 
> > I don't think so, for two reasons:
> > - I can't think of an initializer that would make sense for a dirent
> 
> The same as it would (implied - usually, e.g. with gmalloc0()) if you were 
> allocating it on heap: by initializing it with all zeroes, e.g. just:
> 
>       struct dirent dent = {};
> 

Ok, so you have a zeroed dent, which is likely just as wrong as
any other arbitrary value... I just agree that if we have some
bug that prevents v9fs_co_readdir() to fill the dentry, this
would prevent arbitrary host data to leak to the guest.

> > - if a future change introduces a branch where dent could be used
> >   uninitialized, I'd rather give a chance to the compiler to bark
> 
> The compiler would reliably bark on using it unitialized if you were about to 
> access it directly within the same function. But that's not the case here. 

Hmm... that's the case with the current code base:

v9fs_do_readdir_with_stat:

        err = v9fs_co_readdir(pdu, fidp, &dent);
        if (err <= 0) {
            break;
        }
        err = v9fs_co_name_to_path(pdu, &fidp->path, dent.d_name, &path);
                                                     ^^^^

v9fs_do_readdir:

        err = v9fs_co_readdir(pdu, fidp, &dent);
        if (err <= 0) {
            break;
        }
        v9fs_string_init(&name);
        v9fs_string_sprintf(&name, "%s", dent.d_name);
                                         ^^^^

> dirent is passed by reference to a function which will be altering it. Such 
> stacked relations usually require more sophisticated diagnostics, like e.g. 
> exeuting the LLVM sanitizer.
> 

We also do Coverity runs on a regular basis.

> Best regards,
> Christian Schoenebeck
> 
>