linux-next: build warnings after merge of the keys tree

linux-next: build warnings after merge of the keys tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Hi all,

After merging the keys tree, today's linux-next build (x86_64
allmodconfig) produced these warnings:

In file included from include/linux/keyctl.h:11,
                 from include/linux/key.h:35,
                 from include/linux/cred.h:13,
                 from fs/verity/signature.c:10:
fs/verity/signature.c: In function 'fsverity_init_signature':
include/uapi/linux/keyctl.h:52:24: warning: passing argument 5 of 'keyring_alloc' makes pointer from integer without a cast [-Wint-conversion]
 #define KEY_POS_SEARCH 0x08000000 /* possessor can find a key in search / search a keyring */
                        ^
fs/verity/signature.c:140:25: note: in expansion of macro 'KEY_POS_SEARCH'
         current_cred(), KEY_POS_SEARCH |
                         ^~~~~~~~~~~~~~
In file included from include/linux/cred.h:13,
                 from fs/verity/signature.c:10:
include/linux/key.h:386:20: note: expected 'struct key_acl *' but argument is of type 'int'
 extern struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
                    ^~~~~~~~~~~~~

Caused by commit

  f802f2b3a991 ("keys: Replace uid/gid/perm permissions checking with an ACL")

interacting with commit

  318ce3c7b2ff ("fs-verity: support builtin file signatures")

from the fsverity tree.

(Have I mentioned that I have API changes? ;-))

I have applied the following merge fix patch:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 30 Jul 2019 12:13:38 +1000
Subject: [PATCH] fsverity: merge fix for keyring_alloc API change

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/verity/signature.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/verity/signature.c b/fs/verity/signature.c
index c8b255232de5..a7aac30c56ae 100644
--- a/fs/verity/signature.c
+++ b/fs/verity/signature.c
@@ -131,15 +131,26 @@ static inline int __init fsverity_sysctl_init(void)
 }
 #endif /* !CONFIG_SYSCTL */
 
+static struct key_acl fsverity_acl = {
+	.usage	= REFCOUNT_INIT(1),
+	.possessor_viewable = true,
+	.nr_ace	= 2,
+	.aces = {
+		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_JOIN |
+				  KEY_ACE_INVAL),
+		KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_WRITE |
+			      KEY_ACE_CLEAR | KEY_ACE_SEARCH |
+			      KEY_ACE_SET_SECURITY | KEY_ACE_REVOKE),
+	}
+};
+
 int __init fsverity_init_signature(void)
 {
 	struct key *ring;
 	int err;
 
 	ring = keyring_alloc(".fs-verity", KUIDT_INIT(0), KGIDT_INIT(0),
-			     current_cred(), KEY_POS_SEARCH |
-				KEY_USR_VIEW | KEY_USR_READ | KEY_USR_WRITE |
-				KEY_USR_SEARCH | KEY_USR_SETATTR,
+			     current_cred(), &fsverity_acl,
 			     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);
 	if (IS_ERR(ring))
 		return PTR_ERR(ring);
-- 
2.20.1

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warnings after merge of the keys tree

[Eric Biggers]

On Tue, Jul 30, 2019 at 12:30:42PM +1000, Stephen Rothwell wrote:
> Subject: [PATCH] fsverity: merge fix for keyring_alloc API change
> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  fs/verity/signature.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/verity/signature.c b/fs/verity/signature.c
> index c8b255232de5..a7aac30c56ae 100644
> --- a/fs/verity/signature.c
> +++ b/fs/verity/signature.c
> @@ -131,15 +131,26 @@ static inline int __init fsverity_sysctl_init(void)
>  }
>  #endif /* !CONFIG_SYSCTL */
>  
> +static struct key_acl fsverity_acl = {
> +	.usage	= REFCOUNT_INIT(1),
> +	.possessor_viewable = true,

I don't think .possessor_viewable should be set here, since there's no
KEY_POSSESSOR_ACE(KEY_ACE_VIEW) in the ACL.  David, this bool is supposed to
mean such an entry is present, right?  Is it really necessary, since it's
redundant with the ACL itself?

> +	.nr_ace	= 2,
> +	.aces = {
> +		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_JOIN |
> +				  KEY_ACE_INVAL),
> +		KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_WRITE |
> +			      KEY_ACE_CLEAR | KEY_ACE_SEARCH |
> +			      KEY_ACE_SET_SECURITY | KEY_ACE_REVOKE),
> +	}
> +};
> +
>  int __init fsverity_init_signature(void)
>  {
>  	struct key *ring;
>  	int err;
>  
>  	ring = keyring_alloc(".fs-verity", KUIDT_INIT(0), KGIDT_INIT(0),
> -			     current_cred(), KEY_POS_SEARCH |
> -				KEY_USR_VIEW | KEY_USR_READ | KEY_USR_WRITE |
> -				KEY_USR_SEARCH | KEY_USR_SETATTR,
> +			     current_cred(), &fsverity_acl,
>  			     KEY_ALLOC_NOT_IN_QUOTA, NULL, NULL);

Otherwise this looks good, thanks Stephen.  I'll want to remove a few of these
permissions in a separate patch later, but for now we can just keep it
equivalent to the original code as you've done.

We'll have the same problem in fs/crypto/ in a week or two if/when I apply
another patch series.  For that one I'll send you a merge resolution so you
don't have to do it yourself...

Thanks,

- Eric

Re: linux-next: build warnings after merge of the keys tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

Hi Eric,

On Mon, 29 Jul 2019 20:47:04 -0700 Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jul 30, 2019 at 12:30:42PM +1000, Stephen Rothwell wrote:
> > +static struct key_acl fsverity_acl = {
> > +	.usage	= REFCOUNT_INIT(1),
> > +	.possessor_viewable = true,  
> 
> I don't think .possessor_viewable should be set here, since there's no
> KEY_POSSESSOR_ACE(KEY_ACE_VIEW) in the ACL.  David, this bool is supposed to
> mean such an entry is present, right?  Is it really necessary, since it's
> redundant with the ACL itself?

OK, I can take that out of the patch for tomorrow.

> Otherwise this looks good, thanks Stephen.  I'll want to remove a few of these
> permissions in a separate patch later, but for now we can just keep it
> equivalent to the original code as you've done.

Thanks for the review.

> We'll have the same problem in fs/crypto/ in a week or two if/when I apply
> another patch series.  For that one I'll send you a merge resolution so you
> don't have to do it yourself...

That will be great, thanks.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warnings after merge of the keys tree

[Eric Biggers]

On Tue, Jul 30, 2019 at 01:52:16PM +1000, Stephen Rothwell wrote:
> Hi Eric,
> 
> On Mon, 29 Jul 2019 20:47:04 -0700 Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 12:30:42PM +1000, Stephen Rothwell wrote:
> > > +static struct key_acl fsverity_acl = {
> > > +	.usage	= REFCOUNT_INIT(1),
> > > +	.possessor_viewable = true,  
> > 
> > I don't think .possessor_viewable should be set here, since there's no
> > KEY_POSSESSOR_ACE(KEY_ACE_VIEW) in the ACL.  David, this bool is supposed to
> > mean such an entry is present, right?  Is it really necessary, since it's
> > redundant with the ACL itself?
> 
> OK, I can take that out of the patch for tomorrow.
> 
> > Otherwise this looks good, thanks Stephen.  I'll want to remove a few of these
> > permissions in a separate patch later, but for now we can just keep it
> > equivalent to the original code as you've done.
> 
> Thanks for the review.
> 

Hmm, apparently it's not *exactly* equivalent, since the ACL is missing INVAL
and JOIN permission for the owner, while those were originally granted by SEARCH
permission.  We don't need those, but just to keep the merge resolution itself
as boring as possible, can you please use the following to make it equivalent:

static struct key_acl fsverity_acl = {
	.usage	= REFCOUNT_INIT(1),
	.nr_ace	= 2,
	.aces = {
		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_JOIN |
				  KEY_ACE_INVAL),
		KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_WRITE |
			      KEY_ACE_SEARCH | KEY_ACE_SET_SECURITY |
			      KEY_ACE_INVAL | KEY_ACE_REVOKE | KEY_ACE_JOIN |
			      KEY_ACE_CLEAR),
	}
};


Thanks!

- Eric

Re: linux-next: build warnings after merge of the keys tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

Hi Eric,

On Tue, 30 Jul 2019 18:40:34 -0700 Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jul 30, 2019 at 01:52:16PM +1000, Stephen Rothwell wrote:
> > Hi Eric,
> > 
> > On Mon, 29 Jul 2019 20:47:04 -0700 Eric Biggers <ebiggers@kernel.org> wrote:  
> > >
> > > On Tue, Jul 30, 2019 at 12:30:42PM +1000, Stephen Rothwell wrote:  
> > > > +static struct key_acl fsverity_acl = {
> > > > +	.usage	= REFCOUNT_INIT(1),
> > > > +	.possessor_viewable = true,    
> > > 
> > > I don't think .possessor_viewable should be set here, since there's no
> > > KEY_POSSESSOR_ACE(KEY_ACE_VIEW) in the ACL.  David, this bool is supposed to
> > > mean such an entry is present, right?  Is it really necessary, since it's
> > > redundant with the ACL itself?  
> > 
> > OK, I can take that out of the patch for tomorrow.
> >   
> > > Otherwise this looks good, thanks Stephen.  I'll want to remove a few of these
> > > permissions in a separate patch later, but for now we can just keep it
> > > equivalent to the original code as you've done.  
> > 
> > Thanks for the review.
> 
> Hmm, apparently it's not *exactly* equivalent, since the ACL is missing INVAL
> and JOIN permission for the owner, while those were originally granted by SEARCH
> permission.  We don't need those, but just to keep the merge resolution itself
> as boring as possible, can you please use the following to make it equivalent:
> 
> static struct key_acl fsverity_acl = {
> 	.usage	= REFCOUNT_INIT(1),
> 	.nr_ace	= 2,
> 	.aces = {
> 		KEY_POSSESSOR_ACE(KEY_ACE_SEARCH | KEY_ACE_JOIN |
> 				  KEY_ACE_INVAL),
> 		KEY_OWNER_ACE(KEY_ACE_VIEW | KEY_ACE_READ | KEY_ACE_WRITE |
> 			      KEY_ACE_SEARCH | KEY_ACE_SET_SECURITY |
> 			      KEY_ACE_INVAL | KEY_ACE_REVOKE | KEY_ACE_JOIN |
> 			      KEY_ACE_CLEAR),
> 	}
> };

OK, I have fixed up the patch for today (not quite as above, but
equivalently since I am editting a patch and I usually get that
wrong :-))

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build warnings after merge of the keys tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

Hi all,

After merging the keys tree, today's linux-next build (x86_64
allmodconfig) produced these warnings:

scripts/Makefile.lib:8: 'always' is deprecated. Please use 'always-y' instead
scripts/Makefile.lib:12: 'hostprogs-y' and 'hostprogs-m' are deprecated. Please use 'hostprogs' instead

Introduced by commit

  631ec151fd96 ("Add sample notification program")

interacting with commit

  ee066c3ddf7b ("kbuild: warn if always, hostprogs-y, or hostprogs-m is used")

from the kbuild tree.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warnings after merge of the keys tree

[David Howells]

I've dropped the keyrings ACL patches for now and repushed the branch.  I'll
wait till after the merge window before pushing them again (with their fixes).

David

linux-next: build warnings after merge of the keys tree

[Stephen Rothwell]

Hi David,

After merging the keys tree, today's linux-next build (arm
multi_v7_defconfiga) produced these warning:

certs/system_keyring.c:39:23: warning: 'trusted_keyring_acl' defined but not used [-Wunused-variable]
 static struct key_acl trusted_keyring_acl = {
                       ^
net/wireless/reg.c: In function 'load_keys_from_buffer':
net/wireless/reg.c:688:9: warning: passing argument 6 of 'key_create_or_update' makes pointer from integer without a cast [-Wint-conversion]
         ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
         ^
In file included from include/linux/cred.h:17:0,
                 from include/linux/seq_file.h:12,
                 from include/linux/pinctrl/consumer.h:17,
                 from include/linux/pinctrl/devinfo.h:21,
                 from include/linux/device.h:24,
                 from include/linux/platform_device.h:14,
                 from net/wireless/reg.c:55:
include/linux/key.h:297:18: note: expected 'struct key_acl *' but argument is of type 'int'
 extern key_ref_t key_create_or_update(key_ref_t keyring,
                  ^
net/wireless/reg.c: In function 'load_builtin_regdb_keys':
net/wireless/reg.c:715:10: warning: passing argument 5 of 'keyring_alloc' makes pointer from integer without a cast [-Wint-conversion]
          ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
          ^
In file included from include/linux/cred.h:17:0,
                 from include/linux/seq_file.h:12,
                 from include/linux/pinctrl/consumer.h:17,
                 from include/linux/pinctrl/devinfo.h:21,
                 from include/linux/device.h:24,
                 from include/linux/platform_device.h:14,
                 from net/wireless/reg.c:55:
include/linux/key.h:315:20: note: expected 'struct key_acl *' but argument is of type 'int'
 extern struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
                    ^

-- 
Cheers,
Stephen Rothwell