Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-12811

lustre_swab_fiemap needs to check fm_mapped_extents

Details

    • Bug
    • Resolution: Fixed
    • Critical
    • Lustre 2.14.0
    • Lustre 2.13.0, Lustre 2.12.3
    • 3
    • 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

      Attachments

        Issue Links

          Activity

            [LU-12811] lustre_swab_fiemap needs to check fm_mapped_extents

            It looks like all of these patches are landed.

            adilger Andreas Dilger added a comment - It looks like all of these patches are landed.

            Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36848/
            Subject: LU-12811 ptlrpc: do some cleanups in swabber_dumper_helper()
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: 4b1ba33d7ab5f122e02b1cf8da4285b2ba798613

            gerrit Gerrit Updater added a comment - Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36848/ Subject: LU-12811 ptlrpc: do some cleanups in swabber_dumper_helper() Project: fs/lustre-release Branch: master Current Patch Set: Commit: 4b1ba33d7ab5f122e02b1cf8da4285b2ba798613

            Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36867/
            Subject: LU-12811 ptlrpc: pass buflen to lustre_swab_object_update_*()
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: c2a6f1a2afee6c416a3e7659b863bcc6246761f0

            gerrit Gerrit Updater added a comment - Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36867/ Subject: LU-12811 ptlrpc: pass buflen to lustre_swab_object_update_*() Project: fs/lustre-release Branch: master Current Patch Set: Commit: c2a6f1a2afee6c416a3e7659b863bcc6246761f0

            Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36435/
            Subject: LU-12811 ptlrpc: pass buffer size to the swabbing functions
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: 6099136315e8f71c52ea720c7622a7c05b8e3640

            gerrit Gerrit Updater added a comment - Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/36435/ Subject: LU-12811 ptlrpc: pass buffer size to the swabbing functions Project: fs/lustre-release Branch: master Current Patch Set: Commit: 6099136315e8f71c52ea720c7622a7c05b8e3640

            Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36867
            Subject: LU-12811 ptlrpc: pass len to lustre_swab_object_update_reply()
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: 82be9af83291efe265171c1779b16493f6e6e180

            gerrit Gerrit Updater added a comment - Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36867 Subject: LU-12811 ptlrpc: pass len to lustre_swab_object_update_reply() Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: 82be9af83291efe265171c1779b16493f6e6e180

            Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36848
            Subject: LU-12811 ptlrpc: do some cleanups in swabber_dumper_helper()
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: 937edd3c3324bc8ce659572a9e1f55c16636f5b4

            gerrit Gerrit Updater added a comment - Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36848 Subject: LU-12811 ptlrpc: do some cleanups in swabber_dumper_helper() Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: 937edd3c3324bc8ce659572a9e1f55c16636f5b4
            adilger Andreas Dilger added a comment - - edited

            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).

            adilger Andreas Dilger added a comment - - edited 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).

            Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36435
            Subject: LU-12811 ptlrpc: add some validation checks
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: a931b21beb6895c9b70b76bb5fc6a3cd069f3b5e

            gerrit Gerrit Updater added a comment - Emoly Liu (emoly@whamcloud.com) uploaded a new patch: https://review.whamcloud.com/36435 Subject: LU-12811 ptlrpc: add some validation checks Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: a931b21beb6895c9b70b76bb5fc6a3cd069f3b5e
            green Oleg Drokin added a comment -

            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.

            green Oleg Drokin added a comment - 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.
            green Oleg Drokin added a comment -

            no.

            green Oleg Drokin added a comment - no.

            People

              emoly.liu Emoly Liu
              green Oleg Drokin
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: