diff mbox series

[net-next,08/12] macsec: use NLA_POLICY_VALIDATE_FN to validate IFLA_MACSEC_CIPHER_SUITE

Message ID 5d4541915e5229c0329ff8e6618439ca21767b18.1664379352.git.sd@queasysnail.net (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series macsec: replace custom netlink attribute checks with policy-level checks | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 4 this patch: 5
netdev/cc_maintainers fail 4 maintainers not CCed: kuba@kernel.org pabeni@redhat.com edumazet@google.com davem@davemloft.net
netdev/build_clang fail Errors and warnings before: 0 this patch: 2
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 4 this patch: 5
netdev/checkpatch warning WARNING: line length of 92 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Sabrina Dubroca Sept. 28, 2022, 4:17 p.m. UTC 
Unfortunately, since the value of MACSEC_DEFAULT_CIPHER_ID doesn't fit
near the others, we can't use a simple range in the policy.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

kernel test robot Sept. 28, 2022, 9:07 p.m. UTC | #1 
Hi Sabrina,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Sabrina-Dubroca/macsec-replace-custom-netlink-attribute-checks-with-policy-level-checks/20220929-003145
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b9a5cbf8ba24e88071a97a51a09ef5cdf0d1f6a1
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a58673e9703907581b22fff5004b3a080d50feeb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sabrina-Dubroca/macsec-replace-custom-netlink-attribute-checks-with-policy-level-checks/20220929-003145
        git checkout a58673e9703907581b22fff5004b3a080d50feeb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/macsec.c: In function 'macsec_validate_attr':
>> drivers/net/macsec.c:4122:13: warning: variable 'csid' set but not used [-Wunused-but-set-variable]
    4122 |         u64 csid = MACSEC_DEFAULT_CIPHER_ID;
         |             ^~~~


vim +/csid +4122 drivers/net/macsec.c

a58673e9703907 Sabrina Dubroca   2022-09-28  4118  
a8b8a889e369de Matthias Schiffer 2017-06-25  4119  static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
a8b8a889e369de Matthias Schiffer 2017-06-25  4120  				struct netlink_ext_ack *extack)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4121  {
748164802c1bd2 Sabrina Dubroca   2016-04-22 @4122  	u64 csid = MACSEC_DEFAULT_CIPHER_ID;
b1671253c60158 Lior Nahmanson    2022-09-05  4123  	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4124  	int flag;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4125  	bool es, scb, sci;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4126  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4127  	if (!data)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4128  		return 0;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4129  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4130  	if (data[IFLA_MACSEC_CIPHER_SUITE])
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4131  		csid = nla_get_u64(data[IFLA_MACSEC_CIPHER_SUITE]);
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4132  
f04c392d2dd97a Davide Caratti    2016-07-22  4133  	if (data[IFLA_MACSEC_ICV_LEN]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4134  		icv_len = nla_get_u8(data[IFLA_MACSEC_ICV_LEN]);
b1671253c60158 Lior Nahmanson    2022-09-05  4135  		if (icv_len != MACSEC_DEFAULT_ICV_LEN) {
f04c392d2dd97a Davide Caratti    2016-07-22  4136  			char dummy_key[DEFAULT_SAK_LEN] = { 0 };
f04c392d2dd97a Davide Caratti    2016-07-22  4137  			struct crypto_aead *dummy_tfm;
f04c392d2dd97a Davide Caratti    2016-07-22  4138  
f04c392d2dd97a Davide Caratti    2016-07-22  4139  			dummy_tfm = macsec_alloc_tfm(dummy_key,
f04c392d2dd97a Davide Caratti    2016-07-22  4140  						     DEFAULT_SAK_LEN,
f04c392d2dd97a Davide Caratti    2016-07-22  4141  						     icv_len);
f04c392d2dd97a Davide Caratti    2016-07-22  4142  			if (IS_ERR(dummy_tfm))
f04c392d2dd97a Davide Caratti    2016-07-22  4143  				return PTR_ERR(dummy_tfm);
f04c392d2dd97a Davide Caratti    2016-07-22  4144  			crypto_free_aead(dummy_tfm);
f04c392d2dd97a Davide Caratti    2016-07-22  4145  		}
f04c392d2dd97a Davide Caratti    2016-07-22  4146  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4147  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4148  	if (data[IFLA_MACSEC_ENCODING_SA]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4149  		if (nla_get_u8(data[IFLA_MACSEC_ENCODING_SA]) >= MACSEC_NUM_AN)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4150  			return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4151  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4152  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4153  	for (flag = IFLA_MACSEC_ENCODING_SA + 1;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4154  	     flag < IFLA_MACSEC_VALIDATION;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4155  	     flag++) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4156  		if (data[flag]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4157  			if (nla_get_u8(data[flag]) > 1)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4158  				return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4159  		}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4160  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4161  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4162  	es  = data[IFLA_MACSEC_ES] ? nla_get_u8(data[IFLA_MACSEC_ES]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4163  	sci = data[IFLA_MACSEC_INC_SCI] ? nla_get_u8(data[IFLA_MACSEC_INC_SCI]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4164  	scb = data[IFLA_MACSEC_SCB] ? nla_get_u8(data[IFLA_MACSEC_SCB]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4165  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4166  	if ((sci && (scb || es)) || (scb && es))
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4167  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4168  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4169  	if (data[IFLA_MACSEC_VALIDATION] &&
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4170  	    nla_get_u8(data[IFLA_MACSEC_VALIDATION]) > MACSEC_VALIDATE_MAX)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4171  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4172  
4b1fb9352f351f Sabrina Dubroca   2016-04-22  4173  	if ((data[IFLA_MACSEC_REPLAY_PROTECT] &&
4b1fb9352f351f Sabrina Dubroca   2016-04-22  4174  	     nla_get_u8(data[IFLA_MACSEC_REPLAY_PROTECT])) &&
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4175  	    !data[IFLA_MACSEC_WINDOW])
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4176  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4177  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4178  	return 0;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4179  }
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4180
kernel test robot Sept. 28, 2022, 9:50 p.m. UTC | #2 
Hi Sabrina,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Sabrina-Dubroca/macsec-replace-custom-netlink-attribute-checks-with-policy-level-checks/20220929-003145
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b9a5cbf8ba24e88071a97a51a09ef5cdf0d1f6a1
config: i386-randconfig-a004
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a58673e9703907581b22fff5004b3a080d50feeb
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sabrina-Dubroca/macsec-replace-custom-netlink-attribute-checks-with-policy-level-checks/20220929-003145
        git checkout a58673e9703907581b22fff5004b3a080d50feeb
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/net/macsec.c:4122:6: warning: variable 'csid' set but not used [-Wunused-but-set-variable]
           u64 csid = MACSEC_DEFAULT_CIPHER_ID;
               ^
   1 warning generated.


vim +/csid +4122 drivers/net/macsec.c

a58673e9703907 Sabrina Dubroca   2022-09-28  4118  
a8b8a889e369de Matthias Schiffer 2017-06-25  4119  static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
a8b8a889e369de Matthias Schiffer 2017-06-25  4120  				struct netlink_ext_ack *extack)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4121  {
748164802c1bd2 Sabrina Dubroca   2016-04-22 @4122  	u64 csid = MACSEC_DEFAULT_CIPHER_ID;
b1671253c60158 Lior Nahmanson    2022-09-05  4123  	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4124  	int flag;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4125  	bool es, scb, sci;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4126  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4127  	if (!data)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4128  		return 0;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4129  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4130  	if (data[IFLA_MACSEC_CIPHER_SUITE])
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4131  		csid = nla_get_u64(data[IFLA_MACSEC_CIPHER_SUITE]);
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4132  
f04c392d2dd97a Davide Caratti    2016-07-22  4133  	if (data[IFLA_MACSEC_ICV_LEN]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4134  		icv_len = nla_get_u8(data[IFLA_MACSEC_ICV_LEN]);
b1671253c60158 Lior Nahmanson    2022-09-05  4135  		if (icv_len != MACSEC_DEFAULT_ICV_LEN) {
f04c392d2dd97a Davide Caratti    2016-07-22  4136  			char dummy_key[DEFAULT_SAK_LEN] = { 0 };
f04c392d2dd97a Davide Caratti    2016-07-22  4137  			struct crypto_aead *dummy_tfm;
f04c392d2dd97a Davide Caratti    2016-07-22  4138  
f04c392d2dd97a Davide Caratti    2016-07-22  4139  			dummy_tfm = macsec_alloc_tfm(dummy_key,
f04c392d2dd97a Davide Caratti    2016-07-22  4140  						     DEFAULT_SAK_LEN,
f04c392d2dd97a Davide Caratti    2016-07-22  4141  						     icv_len);
f04c392d2dd97a Davide Caratti    2016-07-22  4142  			if (IS_ERR(dummy_tfm))
f04c392d2dd97a Davide Caratti    2016-07-22  4143  				return PTR_ERR(dummy_tfm);
f04c392d2dd97a Davide Caratti    2016-07-22  4144  			crypto_free_aead(dummy_tfm);
f04c392d2dd97a Davide Caratti    2016-07-22  4145  		}
f04c392d2dd97a Davide Caratti    2016-07-22  4146  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4147  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4148  	if (data[IFLA_MACSEC_ENCODING_SA]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4149  		if (nla_get_u8(data[IFLA_MACSEC_ENCODING_SA]) >= MACSEC_NUM_AN)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4150  			return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4151  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4152  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4153  	for (flag = IFLA_MACSEC_ENCODING_SA + 1;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4154  	     flag < IFLA_MACSEC_VALIDATION;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4155  	     flag++) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4156  		if (data[flag]) {
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4157  			if (nla_get_u8(data[flag]) > 1)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4158  				return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4159  		}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4160  	}
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4161  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4162  	es  = data[IFLA_MACSEC_ES] ? nla_get_u8(data[IFLA_MACSEC_ES]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4163  	sci = data[IFLA_MACSEC_INC_SCI] ? nla_get_u8(data[IFLA_MACSEC_INC_SCI]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4164  	scb = data[IFLA_MACSEC_SCB] ? nla_get_u8(data[IFLA_MACSEC_SCB]) : false;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4165  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4166  	if ((sci && (scb || es)) || (scb && es))
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4167  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4168  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4169  	if (data[IFLA_MACSEC_VALIDATION] &&
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4170  	    nla_get_u8(data[IFLA_MACSEC_VALIDATION]) > MACSEC_VALIDATE_MAX)
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4171  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4172  
4b1fb9352f351f Sabrina Dubroca   2016-04-22  4173  	if ((data[IFLA_MACSEC_REPLAY_PROTECT] &&
4b1fb9352f351f Sabrina Dubroca   2016-04-22  4174  	     nla_get_u8(data[IFLA_MACSEC_REPLAY_PROTECT])) &&
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4175  	    !data[IFLA_MACSEC_WINDOW])
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4176  		return -EINVAL;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4177  
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4178  	return 0;
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4179  }
c09440f7dcb304 Sabrina Dubroca   2016-03-11  4180
Jakub Kicinski Sept. 29, 2022, 4:08 p.m. UTC | #3 
On Wed, 28 Sep 2022 18:17:21 +0200 Sabrina Dubroca wrote:
> Unfortunately, since the value of MACSEC_DEFAULT_CIPHER_ID doesn't fit
> near the others, we can't use a simple range in the policy.

This one warns: 

drivers/net/macsec.c:4122:6: warning: variable 'csid' set but not used [-Wunused-but-set-variable]
        u64 csid = MACSEC_DEFAULT_CIPHER_ID;
            ^
diff mbox series

Patch

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index c70dd40e9d8d..3863f41c9106 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3645,11 +3645,13 @@  static const struct device_type macsec_type = {
 	.name = "macsec",
 };
 
+static int validate_cipher_suite(const struct nlattr *attr,
+				 struct netlink_ext_ack *extack);
 static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_SCI] = { .type = NLA_U64 },
 	[IFLA_MACSEC_PORT] = { .type = NLA_U16 },
 	[IFLA_MACSEC_ICV_LEN] = NLA_POLICY_RANGE(NLA_U8, MACSEC_MIN_ICV_LEN, MACSEC_STD_ICV_LEN),
-	[IFLA_MACSEC_CIPHER_SUITE] = { .type = NLA_U64 },
+	[IFLA_MACSEC_CIPHER_SUITE] = NLA_POLICY_VALIDATE_FN(NLA_U64, validate_cipher_suite),
 	[IFLA_MACSEC_WINDOW] = { .type = NLA_U32 },
 	[IFLA_MACSEC_ENCODING_SA] = { .type = NLA_U8 },
 	[IFLA_MACSEC_ENCRYPT] = { .type = NLA_U8 },
@@ -4099,6 +4101,21 @@  static int macsec_newlink(struct net *net, struct net_device *dev,
 	return err;
 }
 
+static int validate_cipher_suite(const struct nlattr *attr,
+				 struct netlink_ext_ack *extack)
+{
+	switch (nla_get_u64(attr)) {
+	case MACSEC_CIPHER_ID_GCM_AES_128:
+	case MACSEC_CIPHER_ID_GCM_AES_256:
+	case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
+	case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
+	case MACSEC_DEFAULT_CIPHER_ID:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 				struct netlink_ext_ack *extack)
 {
@@ -4128,17 +4145,6 @@  static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 		}
 	}
 
-	switch (csid) {
-	case MACSEC_CIPHER_ID_GCM_AES_128:
-	case MACSEC_CIPHER_ID_GCM_AES_256:
-	case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
-	case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
-	case MACSEC_DEFAULT_CIPHER_ID:
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	if (data[IFLA_MACSEC_ENCODING_SA]) {
 		if (nla_get_u8(data[IFLA_MACSEC_ENCODING_SA]) >= MACSEC_NUM_AN)
 			return -EINVAL;