Re: [PATCH v6 3/5] test: add new driver_data load tester

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Fri, May 12, 2017 at 05:59:23PM +0200, Luis R. Rodriguez wrote:
> On Fri, May 12, 2017 at 09:28:47AM +0900, AKASHI Takahiro wrote:
> > On Thu, May 11, 2017 at 11:32:30AM -0700, Luis R. Rodriguez wrote:
> > > On Thu, May 11, 2017 at 11:26 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > >
> > > > It would seems to make sense to me to only need to verify files when read
> > > > for the first time, once its cache I don't see why we would re-verify them ?
> > > 
> > > To be clear, the fw cache feature reads the files from the fs prior to
> > > suspend, and then uses the in-memory cache on resume. So it would make
> > > sense to me only to rely on fw verification on resume then when the fw
> > > cache is used ?
> > 
> > Good point. I was thinking of need for verification on resume.
> 
> From what we have discussed so far it would seem to me only necessary
> for a sig_check_ok (if we accept a file can have only one signature
> requirement) for a cache entry, and if its not set but a lookup needs
> a sig check it can do a full fs lookup. If such a lookup succeeded
> then it can fill the sig_check_ok in, provided the file contents
> match of course, given the file could have changed under the hood
> between the last file cache lookup (if the file did change that puts
> us at odd with the first lookup, but since its an update and no sig
> check is required, I guess it is fine to use its contents).
> 
> > As cache is not protected 
> 
> Cache should be protected, it should be const and if its not we should fix that.

Yeah, but

> > and visible to the kernel,
> 
> You mean it is visible to the kernel ?

your current implementation doesn't provide any write protection.

> >  some malware might want to rewrite it :)
> 
> Right, we want to be pedantic about that sort of stuff and signature
> verification can help here but those benefits should carry their own
> weight. We should do what we can without file signature verification to
> protect the cache.
> 
> The cache is short lived though, it exists only during suspend/resume.

I found out why my test cases fail:
trigger_config_sync() in test_driver_data.c always enables REQ_KEEP flag
and so cached data (firmware_buf->data) has not been cleaned up.
I haven't fixed it in my test environment although I pointed it out before.

But the issue on write protection is still there wehn REQ_KEEP is used.

Thanks,
-Takahiro AKASHI

>   Luis