[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop
From: |
Greg Kurz |
Subject: |
Re: [PATCH] 9pfs: Fix potential deadlock of QEMU mainloop |
Date: |
Thu, 7 May 2020 15:42:53 +0200 |
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
>
>