| Message ID | 63722af5-2d8d-2455-17ee-988defd3126f@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: fix boundary test in xfs_attr_shortform_verify | expand |
On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote: > The boundary test for the fixed-offset parts of xfs_attr_sf_entry > in xfs_attr_shortform_verify is off by one. endp is the address > just past the end of the valid data; to check the last byte of > a structure at offset of size "size" we must subtract one. > (i.e. for an object at offset 10, size 4, last byte is 13 not 14). > > This can be shown by: > > # touch file > # setfattr -n root.a file > > and subsequent verifications will fail when it's reread from disk. > > This only matters for a last attribute which has a single-byte name > and no value, otherwise the combination of namelen & valuelen will > push endp out and this test won't fail. > > Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs") > Signed-off-by: Eric Sandeen <sandeen@redhat.com> NGGHGHGHG array[1]s that are actually array[0]... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 8623c815164a..a0cf22f0c904 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify( > * Check the fixed-offset parts of the structure are > * within the data buffer. > */ > - if (((char *)sfep + sizeof(*sfep)) >= endp) > + if (((char *)sfep + sizeof(*sfep)-1) >= endp) > return __this_address; > > /* Don't allow names with known bad length. */ >
On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote: > The boundary test for the fixed-offset parts of xfs_attr_sf_entry > in xfs_attr_shortform_verify is off by one. endp is the address > just past the end of the valid data; to check the last byte of > a structure at offset of size "size" we must subtract one. > (i.e. for an object at offset 10, size 4, last byte is 13 not 14). > > This can be shown by: > > # touch file > # setfattr -n root.a file > > and subsequent verifications will fail when it's reread from disk. > > This only matters for a last attribute which has a single-byte name > and no value, otherwise the combination of namelen & valuelen will > push endp out and this test won't fail. > > Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs") > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > index 8623c815164a..a0cf22f0c904 100644 > --- a/fs/xfs/libxfs/xfs_attr_leaf.c > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify( > * Check the fixed-offset parts of the structure are > * within the data buffer. > */ > - if (((char *)sfep + sizeof(*sfep)) >= endp) > + if (((char *)sfep + sizeof(*sfep)-1) >= endp) whitespace? And a comment explaining the magic "- 1" would be nice. Did you audit the code for other occurrences of this same problem? Cheers, Dave.
On 8/25/20 5:41 PM, Dave Chinner wrote: > On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote: >> The boundary test for the fixed-offset parts of xfs_attr_sf_entry >> in xfs_attr_shortform_verify is off by one. endp is the address >> just past the end of the valid data; to check the last byte of >> a structure at offset of size "size" we must subtract one. >> (i.e. for an object at offset 10, size 4, last byte is 13 not 14). >> >> This can be shown by: >> >> # touch file >> # setfattr -n root.a file >> >> and subsequent verifications will fail when it's reread from disk. >> >> This only matters for a last attribute which has a single-byte name >> and no value, otherwise the combination of namelen & valuelen will >> push endp out and this test won't fail. >> >> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs") >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c >> index 8623c815164a..a0cf22f0c904 100644 >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c >> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify( >> * Check the fixed-offset parts of the structure are >> * within the data buffer. >> */ >> - if (((char *)sfep + sizeof(*sfep)) >= endp) >> + if (((char *)sfep + sizeof(*sfep)-1) >= endp) > > whitespace? And a comment explaining the magic "- 1" would be nice. I was following the whitespace example in the various similar macros i.e. XFS_ATTR_SF_ENTSIZE but if people want spaces that's fine by me. :) ditto for degree of commenting on magical -1's; on the one hand it's a common usage. On the other hand, we often get it wrong so a comment probably would help. > Did you audit the code for other occurrences of this same problem? No. I should do that, good point. Now I do wonder if /* * Check that the variable-length part of the structure is * within the data buffer. The next entry starts after the * name component, so nextentry is an acceptable test. */ next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep); if ((char *)next_sfep > endp) return __this_address; should be >= but I'll have to unravel all the macros to see. In that case though the missing "=" makes it too lenient not too strict, at least. In general though, auditing for proper "offset + length [-1] >[=] $THING" where $THING may be last byte or one-past-last-byte is a few days of work, because we have no real consistency about how we do these things and it requires lots of code-reading to get all the context and knowledge of how we're counting. Not really trying to make excuses but I did want to get the demonstrable flaw fixed fairly quickly. Thanks though, these are good points. -Eric > Cheers, > > Dave. >
On Wed, Aug 26, 2020 at 09:32:13AM -0500, Eric Sandeen wrote: > On 8/25/20 5:41 PM, Dave Chinner wrote: > > On Tue, Aug 25, 2020 at 03:25:29PM -0500, Eric Sandeen wrote: > >> The boundary test for the fixed-offset parts of xfs_attr_sf_entry > >> in xfs_attr_shortform_verify is off by one. endp is the address > >> just past the end of the valid data; to check the last byte of > >> a structure at offset of size "size" we must subtract one. > >> (i.e. for an object at offset 10, size 4, last byte is 13 not 14). > >> > >> This can be shown by: > >> > >> # touch file > >> # setfattr -n root.a file > >> > >> and subsequent verifications will fail when it's reread from disk. > >> > >> This only matters for a last attribute which has a single-byte name > >> and no value, otherwise the combination of namelen & valuelen will > >> push endp out and this test won't fail. > >> > >> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs") > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > >> > >> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c > >> index 8623c815164a..a0cf22f0c904 100644 > >> --- a/fs/xfs/libxfs/xfs_attr_leaf.c > >> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c > >> @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify( > >> * Check the fixed-offset parts of the structure are > >> * within the data buffer. > >> */ > >> - if (((char *)sfep + sizeof(*sfep)) >= endp) > >> + if (((char *)sfep + sizeof(*sfep)-1) >= endp) > > > > whitespace? And a comment explaining the magic "- 1" would be nice. > > I was following the whitespace example in the various similar macros > i.e. XFS_ATTR_SF_ENTSIZE but if people want spaces that's fine by me. :) > > ditto for degree of commenting on magical -1's; on the one hand it's a > common usage. On the other hand, we often get it wrong so a comment > probably would help. > > > Did you audit the code for other occurrences of this same problem? TBH I think this ought to be fixed by changing the declaration of xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern fugly macros like struct_sizeof() to calculate the entry sizes without us all having to remember to subtract one from the struct size. > No. I should do that, good point. Now I do wonder if > > /* > * Check that the variable-length part of the structure is > * within the data buffer. The next entry starts after the > * name component, so nextentry is an acceptable test. > */ > next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep); > if ((char *)next_sfep > endp) > return __this_address; > > should be >= but I'll have to unravel all the macros to see. In that case > though the missing "=" makes it too lenient not too strict, at least. *endp points to the first byte after the end of the buffer, because it is defined as (*sfp + size). The end of the last *sfep in the sf attr struct is supposed to coincide with the end of the buffer, so changing this to >= is not correct. --D > In general though, auditing for proper "offset + length [-1] >[=] $THING" > > where $THING may be last byte or one-past-last-byte is a few days of work, because > we have no real consistency about how we do these things and it requires lots of > code-reading to get all the context and knowledge of how we're counting. > > Not really trying to make excuses but I did want to get the demonstrable > flaw fixed fairly quickly. > > Thanks though, these are good points. > > -Eric > > > Cheers, > > > > Dave. > >
On 8/26/20 10:13 AM, Darrick J. Wong wrote: ... > TBH I think this ought to be fixed by changing the declaration of > xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern > fugly macros like struct_sizeof() to calculate the entry sizes without > us all having to remember to subtract one from the struct size. Fair, but I think that in the interest of time we should fix it up with a -1 which is consistent with the other bits of attr code first, then this can all be cleaned up by making it a [] not [1], dropping the magical -1, turning the macros into functions ala dir2, etc. Sound ok? >> No. I should do that, good point. Now I do wonder if >> >> /* >> * Check that the variable-length part of the structure is >> * within the data buffer. The next entry starts after the >> * name component, so nextentry is an acceptable test. >> */ >> next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep); >> if ((char *)next_sfep > endp) >> return __this_address; >> >> should be >= but I'll have to unravel all the macros to see. In that case >> though the missing "=" makes it too lenient not too strict, at least. > > *endp points to the first byte after the end of the buffer, because it > is defined as (*sfp + size). The end of the last *sfep in the sf attr > struct is supposed to coincide with the end of the buffer, so changing > this to >= is not correct. Let me think on that a little more ;) -Eric
On Wed, Aug 26, 2020 at 10:39:26AM -0500, Eric Sandeen wrote: > On 8/26/20 10:13 AM, Darrick J. Wong wrote: > > ... > > > TBH I think this ought to be fixed by changing the declaration of > > xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern > > fugly macros like struct_sizeof() to calculate the entry sizes without > > us all having to remember to subtract one from the struct size. > > Fair, but I think that in the interest of time we should fix it up with a -1 > which is consistent with the other bits of attr code first, then this can all > be cleaned up by making it a [] not [1], dropping the magical -1, turning > the macros into functions ala dir2, etc. > > Sound ok? Yes. sorry, I thought I was suggesting that we start with the quick -1 fix and move on to fixing the struct, but ENOCOFFEE and LPC sessions start too early... :( --d > >> No. I should do that, good point. Now I do wonder if > >> > >> /* > >> * Check that the variable-length part of the structure is > >> * within the data buffer. The next entry starts after the > >> * name component, so nextentry is an acceptable test. > >> */ > >> next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep); > >> if ((char *)next_sfep > endp) > >> return __this_address; > >> > >> should be >= but I'll have to unravel all the macros to see. In that case > >> though the missing "=" makes it too lenient not too strict, at least. > > > > *endp points to the first byte after the end of the buffer, because it > > is defined as (*sfp + size). The end of the last *sfep in the sf attr > > struct is supposed to coincide with the end of the buffer, so changing > > this to >= is not correct. > > Let me think on that a little more ;) > > -Eric
On Wed, Aug 26, 2020 at 08:13:00AM -0700, Darrick J. Wong wrote: > > ditto for degree of commenting on magical -1's; on the one hand it's a > > common usage. On the other hand, we often get it wrong so a comment > > probably would help. > > > > > Did you audit the code for other occurrences of this same problem? > > TBH I think this ought to be fixed by changing the declaration of > xfs_attr_sf_entry.nameval to "uint8_t nameval[]" and using more modern > fugly macros like struct_sizeof() to calculate the entry sizes without > us all having to remember to subtract one from the struct size. Agreed that we absoutely need to do that. It might be worth to have the "simple" fix as a backportable small patch first, though.
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 8623c815164a..a0cf22f0c904 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -1037,7 +1037,7 @@ xfs_attr_shortform_verify( * Check the fixed-offset parts of the structure are * within the data buffer. */ - if (((char *)sfep + sizeof(*sfep)) >= endp) + if (((char *)sfep + sizeof(*sfep)-1) >= endp) return __this_address; /* Don't allow names with known bad length. */
The boundary test for the fixed-offset parts of xfs_attr_sf_entry in xfs_attr_shortform_verify is off by one. endp is the address just past the end of the valid data; to check the last byte of a structure at offset of size "size" we must subtract one. (i.e. for an object at offset 10, size 4, last byte is 13 not 14). This can be shown by: # touch file # setfattr -n root.a file and subsequent verifications will fail when it's reread from disk. This only matters for a last attribute which has a single-byte name and no value, otherwise the combination of namelen & valuelen will push endp out and this test won't fail. Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs") Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---