[LU-12811] lustre_swab_fiemap needs to check fm_mapped_extents Created: 27/Sep/19 Updated: 22/May/20 Resolved: 15/May/20 |
|
| Status: | Resolved |
| Project: | Lustre |
| Component/s: | None |
| Affects Version/s: | Lustre 2.13.0, Lustre 2.12.3 |
| Fix Version/s: | Lustre 2.14.0 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Oleg Drokin | Assignee: | Emoly Liu |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | LTS12 | ||
| Issue Links: |
|
||||||||||||
| Severity: | 3 | ||||||||||||
| Rank (Obsolete): | 9223372036854775807 | ||||||||||||
| Description |
|
It looks like lustre_swab_fiemap can go beyond request buffer as it uses extent count number from network without any checking at face value similar problem exist at lustre_swab_lmv_mds_md_v1 for lmm1->lmv_stripe_count lustre_swab_object_update for ou->ou_params_count |
| Comments |
| Comment by Peter Jones [ 28/Sep/19 ] |
| Comment by Oleg Drokin [ 01/Oct/19 ] |
|
no. |
| Comment by Oleg Drokin [ 08/Oct/19 ] |
|
The biggest obstacle to us doing the correct testing here is that we do not pass in the buffer size to swabbing functions. This is ok for fixed-size buffers, but once we start having variable-sized ones, we are in a bit of trouble. Naturally changing all swab functions to take a size argument will be a huge patch and is not necessary necessary. Now actually looking at the code, we do have variable arrays support denoted by RMF_F_STRUCT_ARRAY flag. So I wonder if we can actually make this work with a field that consists of a static header and then the array. Potentially we need to create a new field type to describe this and it would only be used where needed that way we don't need to update every swab function with the length argument. |
| Comment by Gerrit Updater [ 12/Oct/19 ] |
|
Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36435 |
| Comment by Andreas Dilger [ 17/Oct/19 ] |
|
One option for passing the length to the swabbing function without having to change all of the swabbers at once would be to add a separate rmf_swab_len function pointer to req_msg_field:
int (*rmf_swab_len)(void *, len);
and assign this with a new DEFINE_MSGFL() macro, similar to DEFINE_MSGF(). Then, in swabber_dumper_helper() if the ->rmf_swab_len() function is available, that should be used in preference to ->rmf_swabber() and can pass the buffer length ("len" in that function) to lustre_swab_fiemap() (and the other similar functions) with an extra "u32 len" argument. That allows proper verification of the fields and avoids potential overflow. Unfortunately, there is no way to return an error from those functions (void functions all the way up), so the best we can do is limit the swabbing to the fields that fit within the buffer length and then return -EOVERFLOW. While the error itself will be ignored by the caller, we may as well make the new function prototype return the error as the starting point to fixing the rest of the code to handle errors better in the future. Also, the "LASSERT((len % field->rmf_size) == 0)" in swabber_dumper_helper() should be removed, but I'm not sure what to do in case of an error at that point, since there is the same inability to return an error to the caller and there are too many callers to req_capsule.*_get() to change easily. One option would be to also change the swabber_dumper_helper() to print a CERROR() (so at least we know about the problem) and then return -EPROTO to the caller (which will also be ignored, but again improving the code for the future). |
| Comment by Gerrit Updater [ 25/Nov/19 ] |
|
Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36848 |
| Comment by Gerrit Updater [ 26/Nov/19 ] |
|
Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36867 |
| Comment by Gerrit Updater [ 25/Feb/20 ] |
|
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36435/ |
| Comment by Gerrit Updater [ 11/Mar/20 ] |
|
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36867/ |
| Comment by Gerrit Updater [ 17/Mar/20 ] |
|
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36848/ |
| Comment by Andreas Dilger [ 15/May/20 ] |
|
It looks like all of these patches are landed. |