On Tue, Jul 25, 2023 at 04:43:48PM -0400, Paul Moore wrote: > On Tue, Jul 11, 2023 at 11:43???PM Fan Wu <wufan@xxxxxxxxxxxxxxxxxxx> wrote: > > On Fri, Jul 07, 2023 at 10:53:45AM -0400, Mike Snitzer wrote: > > ... > > > > Both of your calls to security_bdev_setsecurity() to set your blobs in > > > the bdev are suspect because you're doing so from the verity_ctr(). > > > The mapped_device has 2 dm_table slots (active and inactive). The > > > verity_ctr() becomes part of the inactive slot, there is an extra step > > > to bind the inactive table to the active table. > > > > > > This leads to you changing the blobs in the global bdev _before_ the > > > table is actually active. It is possible that the inactive table will > > > simply be removed and the DM verity device put back in service; > > > leaving your blob(s) in the bdev inconsistent. > > > > > > This issue has parallels to how we need to defer changing the global > > > queue_limits associated with a request_queue until _after_ all table > > > loading is settled and then the update is done just before resuming > > > the DM device (mapped_device) -- see dm_table_set_restrictions(). > > > > > > Unfortunately, this feels like it may require a new hook in the > > > target_type struct (e.g. ->finalize()) > > > > Thanks for pointing out this issue. We were calling security_bdev_setsecurity() > > because the roothash signature data is only available in verity_ctr() > > and it is discarded after verity_ctr() finishes. > > After digging deeper into the table_load, I realized that we were indeed > > wrong here. > > > > Based on my understanding of your suggestion, it seems that the correct > > approach would be to save the roothash signature into the struct dm_target > Sorry for the delay in responding. It took me a while to test out the design idea suggested by Mike. The current implementation is indeed incorrect. However, I've been able to develop a working prototype that addresses the problem identified in the existing implementation. I still need some additional time to fine-tune and clean up the prototype. My goal is to have everything ready and send it out next month. > Would you be doing this with a LSM hook, or would this live in the > device mapper layer? > In my implemention, it is a new hook in the device mapper layer. The hook is triggered just before activating an inactive table of a mapped device. So in our case, we use the hook to attached the dm-verity's roothash metadata to the block_device struct of mapped device. > > and then invoke security_bdev_setsecurity() before activating > > the inactive table in the __bind function (where dm_table_set_restrictions is called). > > > > To facilitate this process, it seems appropriate to introduce a new hook > > called finalize() within the struct target_type. This hook would enable > > targets to define tasks that need to be completed before activating > > a new table. > > > > In our specific case, we would add a finalize hook to the dm-verity module, > > allowing us to call security_bdev_setsecurity() and associate the roothash > > information in the struct dm_target with the struct block_device of > > the struct mapped_device. Is this correct? > > Where would the finalize() hook be called? It is in the __bind function in drivers/md/dm.c, calling just before rcu_assign_pointer(md->map, (void *)t) which activates the inactive table. -Fan
