Message ID | 20230222131211.3898066-3-tudor.ambarus@linaro.org |
---|---|
State | New |
Headers | show |
Series | ext4: fsmap: Improve key validation | expand |
On Wed, Feb 22, 2023 at 01:12:10PM +0000, Tudor Ambarus wrote: > Sanity checks should be done the soonest possible to avoid superfluous > computations when user provides wrong data. Gather all the checks on > user provided data in a single method and call it immediately after > copying the data from user. This patch changes the validation criteria, moves chunks of code around, and constifies parameters all at once. And all you say here is that you're moving validation code up in the sequence! Also, how does moving callsites around improve things? Do the fstests still pass? > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++------------- > fs/ext4/fsmap.h | 3 +++ > fs/ext4/ioctl.c | 17 +++------------- > 3 files changed, 44 insertions(+), 28 deletions(-) > > diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c > index b5289378a761..a27d9f0967b7 100644 > --- a/fs/ext4/fsmap.c > +++ b/fs/ext4/fsmap.c > @@ -9,6 +9,7 @@ > #include "fsmap.h" > #include "mballoc.h" > #include <linux/sort.h> > +#include <linux/string.h> > #include <linux/list_sort.h> > #include <trace/events/ext4.h> > > @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb, > > /* Do we recognize the device? */ > static bool ext4_getfsmap_is_valid_device(struct super_block *sb, > - struct ext4_fsmap *fm) > + const struct fsmap *fm) > { > if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX || > fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev)) > @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb, > } > > /* Ensure that the low key is less than the high key. */ > -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, > - struct ext4_fsmap *high_key) > +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key, > + const struct fsmap *high_key) > { > + u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length; > + > if (low_key->fmr_device > high_key->fmr_device) > return false; > if (low_key->fmr_device < high_key->fmr_device) > return true; > > - if (low_key->fmr_physical > high_key->fmr_physical) > + if (l_fmr_phys > high_key->fmr_physical) > return false; > - if (low_key->fmr_physical < high_key->fmr_physical) > + if (l_fmr_phys < high_key->fmr_physical) Why are you changing the comparison here? > return true; > > if (low_key->fmr_owner > high_key->fmr_owner) > @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, > return false; > } > > +int ext4_fsmap_check_head(struct super_block *sb, > + const struct fsmap_head *head) > +{ > + const struct fsmap *l = &head->fmh_keys[0]; > + const struct fsmap *h = &head->fmh_keys[1]; > + > + if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) || > + memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) || > + memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved))) > + return -EINVAL; > + /* > + * ext4 doesn't report file extents at all, so the only valid > + * file offsets are the magic ones (all zeroes or all ones). > + */ > + if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL)) > + return -EINVAL; > + > + if (head->fmh_iflags & ~FMH_IF_VALID) > + return -EINVAL; > + > + if (!ext4_getfsmap_is_valid_device(sb, l) || > + !ext4_getfsmap_is_valid_device(sb, h)) > + return -EINVAL; > + > + if (!ext4_getfsmap_check_keys(l, h)) > + return -EINVAL; > + > + return 0; > +} > + > #define EXT4_GETFSMAP_DEVS 2 > /* > * Get filesystem's extents as described in head, and format for > @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, > int i; > int error = 0; > > - if (head->fmh_iflags & ~FMH_IF_VALID) > - return -EINVAL; > - if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) || > - !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1])) > - return -EINVAL; > - > head->fmh_entries = 0; > > /* Set up our device handlers. */ > @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, > dkeys[0].fmr_length = 0; > memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap)); > > - if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1])) > - return -EINVAL; And why is it ok to turn validation of dkeys[0] vs. fmh_keys[1] into a validation of fmh_keys[0..1] ? I guess that's why check_keys now adds the low key physical offset and length? But why not leave the key checks the where they are, since it's dkeys[0..1] that get passed around the implementations? --D > - > info.gfi_next_fsblk = head->fmh_keys[0].fmr_physical + > head->fmh_keys[0].fmr_length; > info.gfi_formatter = formatter; > diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h > index ac642be2302e..8325258def7b 100644 > --- a/fs/ext4/fsmap.h > +++ b/fs/ext4/fsmap.h > @@ -8,6 +8,7 @@ > #define __EXT4_FSMAP_H__ > > struct fsmap; > +struct fsmap_head; > > /* internal fsmap representation */ > struct ext4_fsmap { > @@ -32,6 +33,8 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest, > struct ext4_fsmap *src); > void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest, > struct fsmap *src); > +int ext4_fsmap_check_head(struct super_block *sb, > + const struct fsmap_head *head); > > /* fsmap to userspace formatter - copy to user & advance pointer */ > typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *); > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c > index 8067ccda34e4..eb0ecb012e6a 100644 > --- a/fs/ext4/ioctl.c > +++ b/fs/ext4/ioctl.c > @@ -874,20 +874,9 @@ static int ext4_ioc_getfsmap(struct super_block *sb, > > if (copy_from_user(&head, arg, sizeof(struct fsmap_head))) > return -EFAULT; > - if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) || > - memchr_inv(head.fmh_keys[0].fmr_reserved, 0, > - sizeof(head.fmh_keys[0].fmr_reserved)) || > - memchr_inv(head.fmh_keys[1].fmr_reserved, 0, > - sizeof(head.fmh_keys[1].fmr_reserved))) > - return -EINVAL; > - /* > - * ext4 doesn't report file extents at all, so the only valid > - * file offsets are the magic ones (all zeroes or all ones). > - */ > - if (head.fmh_keys[0].fmr_offset || > - (head.fmh_keys[1].fmr_offset != 0 && > - head.fmh_keys[1].fmr_offset != -1ULL)) > - return -EINVAL; > + error = ext4_fsmap_check_head(sb, &head); > + if (error) > + return error; > > xhead.fmh_iflags = head.fmh_iflags; > xhead.fmh_count = head.fmh_count; > -- > 2.39.2.637.g21b0678d19-goog >
Hi, Darrick, Thanks for taking the time to review this patch. On 3/4/23 02:56, Darrick J. Wong wrote: > On Wed, Feb 22, 2023 at 01:12:10PM +0000, Tudor Ambarus wrote: >> Sanity checks should be done the soonest possible to avoid superfluous >> computations when user provides wrong data. Gather all the checks on >> user provided data in a single method and call it immediately after >> copying the data from user. > > This patch changes the validation criteria, moves chunks of code around, The validation criteria remains the same, there's no functional change in the code. > and constifies parameters all at once. And all you say here is that > you're moving validation code up in the sequence! My apologies, I should have mentioned something about the constification. I chose to do the validation over const data because the data should not be changed at validation time, otherwise one may end with nasty implications on the sequence of validation. The const change deserved at least a comment if not a dedicated patch, I agree. > > Also, how does moving callsites around improve things? Do the fstests You don't waste CPU cycles in case the validation fails later on in the code. Every initialization that is done before the last validation check is superfluous in case the validation fails. Also, having the validation scattered around copies of user data and in different methods is harder to follow. What I did was to gather all validation checks in a single method and call it the soonest possible. IMO this makes the code cleaner and easier to understand. > still pass? Yes, please check the cover letter at: https://lore.kernel.org/linux-ext4/20230222131211.3898066-1-tudor.ambarus@linaro.org/ All the available ext4 fsmap tests passed after this patch set. I tested ext4/{027, 028, 029}. > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++------------- >> fs/ext4/fsmap.h | 3 +++ >> fs/ext4/ioctl.c | 17 +++------------- >> 3 files changed, 44 insertions(+), 28 deletions(-) >> >> diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c >> index b5289378a761..a27d9f0967b7 100644 >> --- a/fs/ext4/fsmap.c >> +++ b/fs/ext4/fsmap.c >> @@ -9,6 +9,7 @@ >> #include "fsmap.h" >> #include "mballoc.h" >> #include <linux/sort.h> >> +#include <linux/string.h> >> #include <linux/list_sort.h> >> #include <trace/events/ext4.h> >> >> @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb, >> >> /* Do we recognize the device? */ >> static bool ext4_getfsmap_is_valid_device(struct super_block *sb, >> - struct ext4_fsmap *fm) >> + const struct fsmap *fm) >> { >> if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX || >> fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev)) >> @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb, >> } >> >> /* Ensure that the low key is less than the high key. */ >> -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, >> - struct ext4_fsmap *high_key) >> +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key, >> + const struct fsmap *high_key) >> { >> + u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length; >> + >> if (low_key->fmr_device > high_key->fmr_device) >> return false; >> if (low_key->fmr_device < high_key->fmr_device) >> return true; >> >> - if (low_key->fmr_physical > high_key->fmr_physical) >> + if (l_fmr_phys > high_key->fmr_physical) >> return false; >> - if (low_key->fmr_physical < high_key->fmr_physical) >> + if (l_fmr_phys < high_key->fmr_physical) > > Why are you changing the comparison here? So that I preserve the validation check that was done before this patch. In the code there are 3 representations of the key on which we currently do validations: 1/ the ones from struct fsmap_head head; -> contains the data copied from the user 2/ the ones from struct ext4_fsmap_head xhead; -> ext4 internal representation of the fsmap 3/ dkeys - local keys used to query the device. These are 2/ but with the low key bumped by fmr_length. As you correctly identified below, ext4_getfsmap_check_keys() validated dkeys[0] and fmh_keys[1], so a combination of 2/ and 3/, whereas now I use it to validate directly the data copied from user, thus the data from 1/. In order to do that and at the same time to preserve the logic, I had to introduce a local variable, l_fmr_phys, and bump the low key by fmr_length. As you see, now instead of scattering the checks on data from 1/, 2/ and 3/, I do the checks only on the user provided data, thus 1/. > >> return true; >> >> if (low_key->fmr_owner > high_key->fmr_owner) >> @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, >> return false; >> } >> >> +int ext4_fsmap_check_head(struct super_block *sb, >> + const struct fsmap_head *head) >> +{ >> + const struct fsmap *l = &head->fmh_keys[0]; >> + const struct fsmap *h = &head->fmh_keys[1]; >> + >> + if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) || >> + memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) || >> + memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved))) >> + return -EINVAL; >> + /* >> + * ext4 doesn't report file extents at all, so the only valid >> + * file offsets are the magic ones (all zeroes or all ones). >> + */ >> + if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL)) >> + return -EINVAL; >> + >> + if (head->fmh_iflags & ~FMH_IF_VALID) >> + return -EINVAL; >> + >> + if (!ext4_getfsmap_is_valid_device(sb, l) || >> + !ext4_getfsmap_is_valid_device(sb, h)) >> + return -EINVAL; >> + >> + if (!ext4_getfsmap_check_keys(l, h)) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> #define EXT4_GETFSMAP_DEVS 2 >> /* >> * Get filesystem's extents as described in head, and format for >> @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, >> int i; >> int error = 0; >> >> - if (head->fmh_iflags & ~FMH_IF_VALID) >> - return -EINVAL; >> - if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) || >> - !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1])) >> - return -EINVAL; >> - >> head->fmh_entries = 0; >> >> /* Set up our device handlers. */ >> @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, >> dkeys[0].fmr_length = 0; >> memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap)); >> >> - if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1])) >> - return -EINVAL; > > And why is it ok to turn validation of dkeys[0] vs. fmh_keys[1] into a > validation of fmh_keys[0..1] ? I guess that's why check_keys now adds > the low key physical offset and length? Yes, you're correct. It's okay to validate directly on the data copied from user because 2/ and 3/ are just copies of 1/. > > But why not leave the key checks the where they are, since it's > dkeys[0..1] that get passed around the implementations? > I hope I made it clear at this point. Waiting for your reply. Cheers, ta
diff --git a/fs/ext4/fsmap.c b/fs/ext4/fsmap.c index b5289378a761..a27d9f0967b7 100644 --- a/fs/ext4/fsmap.c +++ b/fs/ext4/fsmap.c @@ -9,6 +9,7 @@ #include "fsmap.h" #include "mballoc.h" #include <linux/sort.h> +#include <linux/string.h> #include <linux/list_sort.h> #include <trace/events/ext4.h> @@ -571,7 +572,7 @@ static int ext4_getfsmap_datadev(struct super_block *sb, /* Do we recognize the device? */ static bool ext4_getfsmap_is_valid_device(struct super_block *sb, - struct ext4_fsmap *fm) + const struct fsmap *fm) { if (fm->fmr_device == 0 || fm->fmr_device == UINT_MAX || fm->fmr_device == new_encode_dev(sb->s_bdev->bd_dev)) @@ -583,17 +584,19 @@ static bool ext4_getfsmap_is_valid_device(struct super_block *sb, } /* Ensure that the low key is less than the high key. */ -static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, - struct ext4_fsmap *high_key) +static bool ext4_getfsmap_check_keys(const struct fsmap *low_key, + const struct fsmap *high_key) { + u64 l_fmr_phys = low_key->fmr_physical + low_key->fmr_length; + if (low_key->fmr_device > high_key->fmr_device) return false; if (low_key->fmr_device < high_key->fmr_device) return true; - if (low_key->fmr_physical > high_key->fmr_physical) + if (l_fmr_phys > high_key->fmr_physical) return false; - if (low_key->fmr_physical < high_key->fmr_physical) + if (l_fmr_phys < high_key->fmr_physical) return true; if (low_key->fmr_owner > high_key->fmr_owner) @@ -604,6 +607,36 @@ static bool ext4_getfsmap_check_keys(struct ext4_fsmap *low_key, return false; } +int ext4_fsmap_check_head(struct super_block *sb, + const struct fsmap_head *head) +{ + const struct fsmap *l = &head->fmh_keys[0]; + const struct fsmap *h = &head->fmh_keys[1]; + + if (memchr_inv(head->fmh_reserved, 0, sizeof(head->fmh_reserved)) || + memchr_inv(l->fmr_reserved, 0, sizeof(l->fmr_reserved)) || + memchr_inv(h->fmr_reserved, 0, sizeof(h->fmr_reserved))) + return -EINVAL; + /* + * ext4 doesn't report file extents at all, so the only valid + * file offsets are the magic ones (all zeroes or all ones). + */ + if (l->fmr_offset || (h->fmr_offset != 0 && h->fmr_offset != -1ULL)) + return -EINVAL; + + if (head->fmh_iflags & ~FMH_IF_VALID) + return -EINVAL; + + if (!ext4_getfsmap_is_valid_device(sb, l) || + !ext4_getfsmap_is_valid_device(sb, h)) + return -EINVAL; + + if (!ext4_getfsmap_check_keys(l, h)) + return -EINVAL; + + return 0; +} + #define EXT4_GETFSMAP_DEVS 2 /* * Get filesystem's extents as described in head, and format for @@ -635,12 +668,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, int i; int error = 0; - if (head->fmh_iflags & ~FMH_IF_VALID) - return -EINVAL; - if (!ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[0]) || - !ext4_getfsmap_is_valid_device(sb, &head->fmh_keys[1])) - return -EINVAL; - head->fmh_entries = 0; /* Set up our device handlers. */ @@ -673,9 +700,6 @@ int ext4_getfsmap(struct super_block *sb, struct ext4_fsmap_head *head, dkeys[0].fmr_length = 0; memset(&dkeys[1], 0xFF, sizeof(struct ext4_fsmap)); - if (!ext4_getfsmap_check_keys(dkeys, &head->fmh_keys[1])) - return -EINVAL; - info.gfi_next_fsblk = head->fmh_keys[0].fmr_physical + head->fmh_keys[0].fmr_length; info.gfi_formatter = formatter; diff --git a/fs/ext4/fsmap.h b/fs/ext4/fsmap.h index ac642be2302e..8325258def7b 100644 --- a/fs/ext4/fsmap.h +++ b/fs/ext4/fsmap.h @@ -8,6 +8,7 @@ #define __EXT4_FSMAP_H__ struct fsmap; +struct fsmap_head; /* internal fsmap representation */ struct ext4_fsmap { @@ -32,6 +33,8 @@ void ext4_fsmap_from_internal(struct super_block *sb, struct fsmap *dest, struct ext4_fsmap *src); void ext4_fsmap_to_internal(struct super_block *sb, struct ext4_fsmap *dest, struct fsmap *src); +int ext4_fsmap_check_head(struct super_block *sb, + const struct fsmap_head *head); /* fsmap to userspace formatter - copy to user & advance pointer */ typedef int (*ext4_fsmap_format_t)(struct ext4_fsmap *, void *); diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 8067ccda34e4..eb0ecb012e6a 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -874,20 +874,9 @@ static int ext4_ioc_getfsmap(struct super_block *sb, if (copy_from_user(&head, arg, sizeof(struct fsmap_head))) return -EFAULT; - if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) || - memchr_inv(head.fmh_keys[0].fmr_reserved, 0, - sizeof(head.fmh_keys[0].fmr_reserved)) || - memchr_inv(head.fmh_keys[1].fmr_reserved, 0, - sizeof(head.fmh_keys[1].fmr_reserved))) - return -EINVAL; - /* - * ext4 doesn't report file extents at all, so the only valid - * file offsets are the magic ones (all zeroes or all ones). - */ - if (head.fmh_keys[0].fmr_offset || - (head.fmh_keys[1].fmr_offset != 0 && - head.fmh_keys[1].fmr_offset != -1ULL)) - return -EINVAL; + error = ext4_fsmap_check_head(sb, &head); + if (error) + return error; xhead.fmh_iflags = head.fmh_iflags; xhead.fmh_count = head.fmh_count;
Sanity checks should be done the soonest possible to avoid superfluous computations when user provides wrong data. Gather all the checks on user provided data in a single method and call it immediately after copying the data from user. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- fs/ext4/fsmap.c | 52 ++++++++++++++++++++++++++++++++++++------------- fs/ext4/fsmap.h | 3 +++ fs/ext4/ioctl.c | 17 +++------------- 3 files changed, 44 insertions(+), 28 deletions(-)