Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640086 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-12 02:30 +0200
    Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-12 18:00 +0200
      Re: [PATCH v6 3/5] test: add new driver_data load tester AKASHI Takahiro <takahiro.akashi@linaro.org> - 2017-05-17 11:10 +0200
        Re: [PATCH v6 3/5] test: add new driver_data load tester "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-17 17:40 +0200

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

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.
As cache is not protected and visible to the kernel, some malware
might want to rewrite it :)

Thanks,
-Takahiro AKASHI

>  Luis

[toc] | [next] | [standalone]

#1640551

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.

> and visible to the kernel,

You mean it is visible to the kernel ?

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

  Luis

[toc] | [prev] | [next] | [standalone]

#1643156

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

[toc] | [prev] | [next] | [standalone]

#1643474

On Wed, May 17, 2017 at 06:08:36PM +0900, AKASHI Takahiro wrote:
> 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.

The cache was implemented long ago by someone other than myself. Patches
are welcomed.

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

Ah, perhaps the later revision of the test driver I posted captured this fix?

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

Cache of firmware is a feature internal to the firmware_class, so it can only
be fixed there. Patches welcomed.

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web