| Message ID | 20190726052724.12338-1-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix extent buffer read/write range checks | expand |
On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: > Several functions to read/write an extent buffer check if specified offset > range resides in the size of the extent buffer. However, those checks have > two problems: > > (1) they don't catch "start == eb->len" case. > (2) it checks offset in extent buffer against logical address using > eb->start. > > Generally, eb->start is much larger than the offset, so the second WARN_ON > was almost useless. > > Fix these problems in read_extent_buffer_to_user(), > {memcmp,write,memzero}_extent_buffer(). > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> Qu already sent similar patch: [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read write functions He centralised the checking code, your >= fixes though should be merged there. > --- > fs/btrfs/extent_io.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 50cbaf1dad5b..c0174f530568 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb, > unsigned long i = (start_offset + start) >> PAGE_SHIFT; > int ret = 0; > > - WARN_ON(start > eb->len); > - WARN_ON(start + len > eb->start + eb->len); > + WARN_ON(start >= eb->len); > + WARN_ON(start + len > eb->len); > > offset = offset_in_page(start_offset + start); > > @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv, > unsigned long i = (start_offset + start) >> PAGE_SHIFT; > int ret = 0; > > - WARN_ON(start > eb->len); > - WARN_ON(start + len > eb->start + eb->len); > + WARN_ON(start >= eb->len); > + WARN_ON(start + len > eb->len); > > offset = offset_in_page(start_offset + start); > > @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv, > size_t start_offset = offset_in_page(eb->start); > unsigned long i = (start_offset + start) >> PAGE_SHIFT; > > - WARN_ON(start > eb->len); > - WARN_ON(start + len > eb->start + eb->len); > + WARN_ON(start >= eb->len); > + WARN_ON(start + len > eb->len); > > offset = offset_in_page(start_offset + start); > > @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start, > size_t start_offset = offset_in_page(eb->start); > unsigned long i = (start_offset + start) >> PAGE_SHIFT; > > - WARN_ON(start > eb->len); > - WARN_ON(start + len > eb->start + eb->len); > + WARN_ON(start >= eb->len); > + WARN_ON(start + len > eb->len); > > offset = offset_in_page(start_offset + start); > >
On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote: > > >On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: >> Several functions to read/write an extent buffer check if specified offset >> range resides in the size of the extent buffer. However, those checks have >> two problems: >> >> (1) they don't catch "start == eb->len" case. >> (2) it checks offset in extent buffer against logical address using >> eb->start. >> >> Generally, eb->start is much larger than the offset, so the second WARN_ON >> was almost useless. >> >> Fix these problems in read_extent_buffer_to_user(), >> {memcmp,write,memzero}_extent_buffer(). >> >> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > >Qu already sent similar patch: > >[PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read >write functions > > >He centralised the checking code, your >= fixes though should be merged >there. Oops, I missed that series. Thank you for pointing out. Then, this should be merged into Qu's version. Qu, could you pick the change from "start > eb->len" to "start >= eb->len"? > > >> --- >> fs/btrfs/extent_io.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 50cbaf1dad5b..c0174f530568 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb, >> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >> int ret = 0; >> >> - WARN_ON(start > eb->len); >> - WARN_ON(start + len > eb->start + eb->len); >> + WARN_ON(start >= eb->len); >> + WARN_ON(start + len > eb->len); >> >> offset = offset_in_page(start_offset + start); >> >> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv, >> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >> int ret = 0; >> >> - WARN_ON(start > eb->len); >> - WARN_ON(start + len > eb->start + eb->len); >> + WARN_ON(start >= eb->len); >> + WARN_ON(start + len > eb->len); >> >> offset = offset_in_page(start_offset + start); >> >> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv, >> size_t start_offset = offset_in_page(eb->start); >> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >> >> - WARN_ON(start > eb->len); >> - WARN_ON(start + len > eb->start + eb->len); >> + WARN_ON(start >= eb->len); >> + WARN_ON(start + len > eb->len); >> >> offset = offset_in_page(start_offset + start); >> >> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start, >> size_t start_offset = offset_in_page(eb->start); >> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >> >> - WARN_ON(start > eb->len); >> - WARN_ON(start + len > eb->start + eb->len); >> + WARN_ON(start >= eb->len); >> + WARN_ON(start + len > eb->len); >> >> offset = offset_in_page(start_offset + start); >> >>
On 2019/7/26 下午2:13, Naohiro Aota wrote: > On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote: >> >> >> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: >>> Several functions to read/write an extent buffer check if specified >>> offset >>> range resides in the size of the extent buffer. However, those checks >>> have >>> two problems: >>> >>> (1) they don't catch "start == eb->len" case. >>> (2) it checks offset in extent buffer against logical address using >>> eb->start. >>> >>> Generally, eb->start is much larger than the offset, so the second >>> WARN_ON >>> was almost useless. >>> >>> Fix these problems in read_extent_buffer_to_user(), >>> {memcmp,write,memzero}_extent_buffer(). >>> >>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >> >> Qu already sent similar patch: >> >> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read >> write functions >> >> >> He centralised the checking code, your >= fixes though should be merged >> there. > > Oops, I missed that series. Thank you for pointing out. Then, this > should be merged into Qu's version. > > Qu, could you pick the change from "start > eb->len" to "start >= eb->len"? start >= eb->len is not always invalid. start == eb->len while len == 0 is still valid. Or should we also warn such bad practice? Thanks, Qu > >> >> >>> --- >>> fs/btrfs/extent_io.c | 16 ++++++++-------- >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>> index 50cbaf1dad5b..c0174f530568 100644 >>> --- a/fs/btrfs/extent_io.c >>> +++ b/fs/btrfs/extent_io.c >>> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct >>> extent_buffer *eb, >>> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >>> int ret = 0; >>> >>> - WARN_ON(start > eb->len); >>> - WARN_ON(start + len > eb->start + eb->len); >>> + WARN_ON(start >= eb->len); >>> + WARN_ON(start + len > eb->len); >>> >>> offset = offset_in_page(start_offset + start); >>> >>> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct >>> extent_buffer *eb, const void *ptrv, >>> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >>> int ret = 0; >>> >>> - WARN_ON(start > eb->len); >>> - WARN_ON(start + len > eb->start + eb->len); >>> + WARN_ON(start >= eb->len); >>> + WARN_ON(start + len > eb->len); >>> >>> offset = offset_in_page(start_offset + start); >>> >>> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer >>> *eb, const void *srcv, >>> size_t start_offset = offset_in_page(eb->start); >>> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >>> >>> - WARN_ON(start > eb->len); >>> - WARN_ON(start + len > eb->start + eb->len); >>> + WARN_ON(start >= eb->len); >>> + WARN_ON(start + len > eb->len); >>> >>> offset = offset_in_page(start_offset + start); >>> >>> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer >>> *eb, unsigned long start, >>> size_t start_offset = offset_in_page(eb->start); >>> unsigned long i = (start_offset + start) >> PAGE_SHIFT; >>> >>> - WARN_ON(start > eb->len); >>> - WARN_ON(start + len > eb->start + eb->len); >>> + WARN_ON(start >= eb->len); >>> + WARN_ON(start + len > eb->len); >>> >>> offset = offset_in_page(start_offset + start); >>> >>>
On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote: > > >On 2019/7/26 下午2:13, Naohiro Aota wrote: >> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote: >>> >>> >>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: >>>> Several functions to read/write an extent buffer check if specified >>>> offset >>>> range resides in the size of the extent buffer. However, those checks >>>> have >>>> two problems: >>>> >>>> (1) they don't catch "start == eb->len" case. >>>> (2) it checks offset in extent buffer against logical address using >>>> eb->start. >>>> >>>> Generally, eb->start is much larger than the offset, so the second >>>> WARN_ON >>>> was almost useless. >>>> >>>> Fix these problems in read_extent_buffer_to_user(), >>>> {memcmp,write,memzero}_extent_buffer(). >>>> >>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >>> >>> Qu already sent similar patch: >>> >>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read >>> write functions >>> >>> >>> He centralised the checking code, your >= fixes though should be merged >>> there. >> >> Oops, I missed that series. Thank you for pointing out. Then, this >> should be merged into Qu's version. >> >> Qu, could you pick the change from "start > eb->len" to "start >= eb->len"? > > start >= eb->len is not always invalid. > > start == eb->len while len == 0 is still valid. Correct. But then, we can even say "start > eb->len" is valid if len == 0? > Or should we also warn such bad practice? Maybe... Or how about let the callers bailing out by e.g. "if (!len) return 1;" in the check function? Regards, Naohiro
On 2019/7/26 下午4:15, Naohiro Aota wrote: > On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote: >> >> >> On 2019/7/26 下午2:13, Naohiro Aota wrote: >>> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote: >>>> >>>> >>>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: >>>>> Several functions to read/write an extent buffer check if specified >>>>> offset >>>>> range resides in the size of the extent buffer. However, those checks >>>>> have >>>>> two problems: >>>>> >>>>> (1) they don't catch "start == eb->len" case. >>>>> (2) it checks offset in extent buffer against logical address using >>>>> eb->start. >>>>> >>>>> Generally, eb->start is much larger than the offset, so the second >>>>> WARN_ON >>>>> was almost useless. >>>>> >>>>> Fix these problems in read_extent_buffer_to_user(), >>>>> {memcmp,write,memzero}_extent_buffer(). >>>>> >>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >>>> >>>> Qu already sent similar patch: >>>> >>>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read >>>> write functions >>>> >>>> >>>> He centralised the checking code, your >= fixes though should be merged >>>> there. >>> >>> Oops, I missed that series. Thank you for pointing out. Then, this >>> should be merged into Qu's version. >>> >>> Qu, could you pick the change from "start > eb->len" to "start >= >>> eb->len"? >> >> start >= eb->len is not always invalid. >> >> start == eb->len while len == 0 is still valid. > > Correct. > > But then, we can even say "start > eb->len" is valid if len == 0? > >> Or should we also warn such bad practice? > > Maybe... > > Or how about let the callers bailing out by e.g. "if (!len) return 1;" > in the check function? Well, let's forgot len == 0 case and make start >= eb->len invalid. That len == 0 is making a lot of invalid use case valid, and making the check more complex. Thanks, Qu > > Regards, > Naohiro
On 2019/7/26 下午4:15, Naohiro Aota wrote: > On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote: >> >> >> On 2019/7/26 下午2:13, Naohiro Aota wrote: >>> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote: >>>> >>>> >>>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote: >>>>> Several functions to read/write an extent buffer check if specified >>>>> offset >>>>> range resides in the size of the extent buffer. However, those checks >>>>> have >>>>> two problems: >>>>> >>>>> (1) they don't catch "start == eb->len" case. >>>>> (2) it checks offset in extent buffer against logical address using >>>>> eb->start. >>>>> >>>>> Generally, eb->start is much larger than the offset, so the second >>>>> WARN_ON >>>>> was almost useless. >>>>> >>>>> Fix these problems in read_extent_buffer_to_user(), >>>>> {memcmp,write,memzero}_extent_buffer(). >>>>> >>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >>>> >>>> Qu already sent similar patch: >>>> >>>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read >>>> write functions >>>> >>>> >>>> He centralised the checking code, your >= fixes though should be merged >>>> there. >>> >>> Oops, I missed that series. Thank you for pointing out. Then, this >>> should be merged into Qu's version. >>> >>> Qu, could you pick the change from "start > eb->len" to "start >= >>> eb->len"? >> >> start >= eb->len is not always invalid. >> >> start == eb->len while len == 0 is still valid. > > Correct. > > But then, we can even say "start > eb->len" is valid if len == 0? Tried the "start >= eb->len" check in the centralized check_eb_range(), and unfortunately it triggers a lot of warnings. Some callers in fact pass start == eb->len and len == 0: memmove_extent_buffer() in btrfs_del_items() copy_extent_buffer() in __push_leaf_*() Since the check of "start > eb->len || len > eb->len || start + len > eb->len)" has already ensured we won't access anything beyond the eb data, I'd prefer to let the start == eb->len && len == 0 case to pass. Doing the extra len == 0 check in those callers seems a little over-reacted IMHO. Thanks, Qu > >> Or should we also warn such bad practice? > > Maybe... > > Or how about let the callers bailing out by e.g. "if (!len) return 1;" > in the check function? > > Regards, > Naohiro
<snip> >> >> But then, we can even say "start > eb->len" is valid if len == 0? > > Tried the "start >= eb->len" check in the centralized check_eb_range(), > and unfortunately it triggers a lot of warnings. > > Some callers in fact pass start == eb->len and len == 0: Isn't this a noop? > memmove_extent_buffer() in btrfs_del_items() > copy_extent_buffer() in __push_leaf_*() > > Since the check of "start > eb->len || len > eb->len || start + len > > eb->len)" has already ensured we won't access anything beyond the eb > data, I'd prefer to let the start == eb->len && len == 0 case to pass. In an ideal world shouldn't callers detect their parameters are going to be a NOOP and never execute the code in the first place? E.g. is it posible that the math in btrfs_del_item is broken for some edge condition hence calling those functions with such parameters? > > Doing the extra len == 0 check in those callers seems a little > over-reacted IMHO. > > Thanks, > Qu >> >>> Or should we also warn such bad practice? >> >> Maybe... >> >> Or how about let the callers bailing out by e.g. "if (!len) return 1;" >> in the check function? >> >> Regards, >> Naohiro >
On 2019/7/29 下午2:46, Nikolay Borisov wrote: > <snip> > >>> >>> But then, we can even say "start > eb->len" is valid if len == 0? >> >> Tried the "start >= eb->len" check in the centralized check_eb_range(), >> and unfortunately it triggers a lot of warnings. >> >> Some callers in fact pass start == eb->len and len == 0: > > Isn't this a noop? Yep. > >> memmove_extent_buffer() in btrfs_del_items() >> copy_extent_buffer() in __push_leaf_*() >> >> Since the check of "start > eb->len || len > eb->len || start + len > >> eb->len)" has already ensured we won't access anything beyond the eb >> data, I'd prefer to let the start == eb->len && len == 0 case to pass. > > In an ideal world shouldn't callers detect their parameters are going to > be a NOOP and never execute the code in the first place? E.g. is it > posible that the math in btrfs_del_item is broken for some edge > condition hence calling those functions with such parameters? This depends. Sometimes we can save unnecessary (len == 0) check depending on how the loop is written. In btrfs, leaf item 0 always ends at eb->len, thus I believe it's the reason why we have some loop generating (start = eb->len len = 0) request. As long as we're not accessing any range beyond [0, eb->len), I tend not to touch all these callers. Thanks, Qu > >> >> Doing the extra len == 0 check in those callers seems a little >> over-reacted IMHO. >> >> Thanks, >> Qu >>> >>>> Or should we also warn such bad practice? >>> >>> Maybe... >>> >>> Or how about let the callers bailing out by e.g. "if (!len) return 1;" >>> in the check function? >>> >>> Regards, >>> Naohiro >>
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 50cbaf1dad5b..c0174f530568 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb, unsigned long i = (start_offset + start) >> PAGE_SHIFT; int ret = 0; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + WARN_ON(start >= eb->len); + WARN_ON(start + len > eb->len); offset = offset_in_page(start_offset + start); @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv, unsigned long i = (start_offset + start) >> PAGE_SHIFT; int ret = 0; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + WARN_ON(start >= eb->len); + WARN_ON(start + len > eb->len); offset = offset_in_page(start_offset + start); @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv, size_t start_offset = offset_in_page(eb->start); unsigned long i = (start_offset + start) >> PAGE_SHIFT; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + WARN_ON(start >= eb->len); + WARN_ON(start + len > eb->len); offset = offset_in_page(start_offset + start); @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start, size_t start_offset = offset_in_page(eb->start); unsigned long i = (start_offset + start) >> PAGE_SHIFT; - WARN_ON(start > eb->len); - WARN_ON(start + len > eb->start + eb->len); + WARN_ON(start >= eb->len); + WARN_ON(start + len > eb->len); offset = offset_in_page(start_offset + start);
Several functions to read/write an extent buffer check if specified offset range resides in the size of the extent buffer. However, those checks have two problems: (1) they don't catch "start == eb->len" case. (2) it checks offset in extent buffer against logical address using eb->start. Generally, eb->start is much larger than the offset, so the second WARN_ON was almost useless. Fix these problems in read_extent_buffer_to_user(), {memcmp,write,memzero}_extent_buffer(). Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/extent_io.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)