On Tue, Aug 16, 2022 at 08:19:35AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 16, 2022 at 10:45:50AM +0200, Carlos Maiolino wrote:
> > Prevent uninitialized data in the stack by initializing getbmap structure
> > to zero.
>
> The kernel should fill out all the bmap[1..BMAP_LEN-1] entries, right?
>
> The only part of the array that's critical to initialize to a known
> value is bmap[0], since that's what the kernel will read to decide what
> to do, right?
>
> Or, zooming out a bit, why did you decide to initialize the struct? Was
> it valgrind complaining about uninitialized ioctl memory, or did someone
> report a bug?
>
I thought about this from a userspace perspective at first, and by pure code
inspection, not anything valgrind complained about.
The previous mentioned patch replaced getbmapx by getbmap to remove the
uninitialized bmv_iflags from the middle of bmap[0], which for sure is the most
critical part here. At the array declaration though, the first element is zeroed,
but it still leaves garbage on the remaining arrays, so, I thought it would be
wise to just zero out the whole array there without leaving uninitialized data
we might trip over in the future.
I just did a quick look from the kernel side, and if I'm not wrong, if the file
has fewer extents than the array has slots, the kernel won't touch the remaining
array entries, leaving that space uninitialized.
I don't think it's a big deal anyway, as xfsdump walk through the array based on
the returned entries.
> (I'm actually fine with this change, I just want to know how you got
> here. ;))
Just thought it as a 'better safe than sorry' kind of situation, nothing more :P
>
> --D
>
> > Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> > ---
> >
> > There is already a patch on the list to remove remaining DMAPI stuff from
> > xfsdump:
> > xfsdump: remove BMV_IF_NO_DMAPI_READ flag
> >
> > This patch though, does not initialize the getbmap structure, and although
> > the
> > first struct in the array is initialized, the remaining structures in the
> > array are not, leaving garbage in the stack.
> >
> >
> > dump/inomap.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/dump/inomap.c b/dump/inomap.c
> > index f3200be..c4ea21d 100644
> > --- a/dump/inomap.c
> > +++ b/dump/inomap.c
> > @@ -1627,7 +1627,7 @@ static off64_t
> > quantity2offset(jdm_fshandle_t *fshandlep, struct xfs_bstat *statp, off64_t qty)
> > {
> > int fd;
> > - struct getbmap bmap[BMAP_LEN];
> > + struct getbmap bmap[BMAP_LEN] = {0};
> > off64_t offset;
> > off64_t offset_next;
> > off64_t qty_accum;
> >
> >
--
Carlos Maiolino
