On Thu, 07 May 2020 13:46:50 +0200 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Mittwoch, 6. Mai 2020 19:54:15 CEST Greg Kurz wrote: > > On Wed, 06 May 2020 15:36:16 +0200 > > > > Christian Schoenebeck <qemu_...@crudebyte.com> 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 > >