| Message ID | 166063952935.40771.5357077583333371260.stgit@orion (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfsdump: Initialize getbmap structure in quantity2offset | expand |
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'm actually fine with this change, I just want to know how you got here. ;)) --D > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > 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; > >
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@redhat.com> > > --- > > > > 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; > > > >
On Wed, Aug 17, 2022 at 10:42:14AM +0200, Carlos Maiolino wrote: > 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. Agreed! > 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. <shrug> In theory, if userspace range-checks itself to bmap[0].bmv_entries then the uninitialized array elements shouldn't be an issue, right? (That said, getbmap* is not an intuitive interface given that it reuses the same struct for the query information and the records, with the result that there are header fields that are ignored, and record fields that are meaningless.) > 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. Right. > I don't think it's a big deal anyway, as xfsdump walk through the array based on > the returned entries. <nod> > > (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 <nod> Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > > > --D > > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > > --- > > > > > > 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
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;
Prevent uninitialized data in the stack by initializing getbmap structure to zero. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- 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(-)