Re: [RFC PATCH v10 11/17] dm-verity: consume root hash digest and signature data via LSM hook

[Fan Wu]

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