| Message ID | 20220525132115.896698-2-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Add support for maps with authenticated values | expand |
Hi Roberto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master horms-ipvs/master net/master net-next/master v5.18 next-20220525]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20220526/202205260057.t4Lmg5Gb-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/196e68e5ddfa50f40efaf20c8df37f3420e38b72
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
git checkout 196e68e5ddfa50f40efaf20c8df37f3420e38b72
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: kernel/bpf/syscall.o: in function `bpf_map_verify_value_sig':
>> syscall.c:(.text+0x4ff): undefined reference to `mod_check_sig'
Hi Roberto,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[also build test ERROR on bpf/master horms-ipvs/master net/master net-next/master v5.18 next-20220525]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20220526/202205260201.H6HGWRhl-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.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/196e68e5ddfa50f40efaf20c8df37f3420e38b72
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
git checkout 196e68e5ddfa50f40efaf20c8df37f3420e38b72
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
kernel/bpf/syscall.c: In function 'bpf_map_verify_value_sig':
>> kernel/bpf/syscall.c:1415:23: error: implicit declaration of function 'verify_pkcs7_signature' [-Werror=implicit-function-declaration]
1415 | ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
| ^~~~~~~~~~~~~~~~~~~~~~
kernel/bpf/syscall.c: At top level:
kernel/bpf/syscall.c:5271:13: warning: no previous prototype for 'unpriv_ebpf_notify' [-Wmissing-prototypes]
5271 | void __weak unpriv_ebpf_notify(int new_state)
| ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/verify_pkcs7_signature +1415 kernel/bpf/syscall.c
1369
1370 int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify)
1371 {
1372 const size_t marker_len = strlen(MODULE_SIG_STRING);
1373 struct module_signature ms;
1374 size_t sig_len;
1375 u32 _modlen;
1376 int ret;
1377
1378 /*
1379 * Format of mod:
1380 *
1381 * verified data+sig size (be32), verified data, sig, unverified data
1382 */
1383 if (modlen <= sizeof(u32))
1384 return -ENOENT;
1385
1386 _modlen = be32_to_cpu(*(u32 *)(mod));
1387
1388 if (_modlen > modlen - sizeof(u32))
1389 return -EINVAL;
1390
1391 modlen = _modlen;
1392 mod += sizeof(u32);
1393
1394 if (modlen <= marker_len)
1395 return -ENOENT;
1396
1397 if (memcmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len))
1398 return -ENOENT;
1399
1400 modlen -= marker_len;
1401
1402 if (modlen <= sizeof(ms))
1403 return -EBADMSG;
1404
1405 memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
1406
1407 ret = mod_check_sig(&ms, modlen, "bpf_map_value");
1408 if (ret)
1409 return ret;
1410
1411 sig_len = be32_to_cpu(ms.sig_len);
1412 modlen -= sig_len + sizeof(ms);
1413
1414 if (verify) {
> 1415 ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
1416 VERIFY_USE_SECONDARY_KEYRING,
1417 VERIFYING_UNSPECIFIED_SIGNATURE,
1418 NULL, NULL);
1419 if (ret < 0)
1420 return ret;
1421 }
1422
1423 return modlen;
1424 }
1425 EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig);
1426
Hi Roberto,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master horms-ipvs/master net/master net-next/master v5.18 next-20220525]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-rhel-8.3-kselftests (https://download.01.org/0day-ci/archive/20220526/202205260606.VXzztn2R-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-14-g5a0004b5-dirty
# https://github.com/intel-lab-lkp/linux/commit/196e68e5ddfa50f40efaf20c8df37f3420e38b72
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Roberto-Sassu/bpf-Add-support-for-maps-with-authenticated-values/20220525-212552
git checkout 196e68e5ddfa50f40efaf20c8df37f3420e38b72
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/bpf/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
kernel/bpf/syscall.c:590:25: sparse: sparse: Using plain integer as NULL pointer
>> kernel/bpf/syscall.c:1386:19: sparse: sparse: cast to restricted __be32
kernel/bpf/syscall.c: note: in included file (through include/linux/bpf.h):
include/linux/bpfptr.h:52:47: sparse: sparse: cast to non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast from non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast to non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast from non-scalar
include/linux/bpfptr.h:81:43: sparse: sparse: cast to non-scalar
include/linux/bpfptr.h:81:43: sparse: sparse: cast from non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast to non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast from non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast to non-scalar
include/linux/bpfptr.h:52:47: sparse: sparse: cast from non-scalar
vim +1386 kernel/bpf/syscall.c
1369
1370 int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify)
1371 {
1372 const size_t marker_len = strlen(MODULE_SIG_STRING);
1373 struct module_signature ms;
1374 size_t sig_len;
1375 u32 _modlen;
1376 int ret;
1377
1378 /*
1379 * Format of mod:
1380 *
1381 * verified data+sig size (be32), verified data, sig, unverified data
1382 */
1383 if (modlen <= sizeof(u32))
1384 return -ENOENT;
1385
> 1386 _modlen = be32_to_cpu(*(u32 *)(mod));
1387
1388 if (_modlen > modlen - sizeof(u32))
1389 return -EINVAL;
1390
1391 modlen = _modlen;
1392 mod += sizeof(u32);
1393
1394 if (modlen <= marker_len)
1395 return -ENOENT;
1396
1397 if (memcmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len))
1398 return -ENOENT;
1399
1400 modlen -= marker_len;
1401
1402 if (modlen <= sizeof(ms))
1403 return -EBADMSG;
1404
1405 memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
1406
1407 ret = mod_check_sig(&ms, modlen, "bpf_map_value");
1408 if (ret)
1409 return ret;
1410
1411 sig_len = be32_to_cpu(ms.sig_len);
1412 modlen -= sig_len + sizeof(ms);
1413
1414 if (verify) {
1415 ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
1416 VERIFY_USE_SECONDARY_KEYRING,
1417 VERIFYING_UNSPECIFIED_SIGNATURE,
1418 NULL, NULL);
1419 if (ret < 0)
1420 return ret;
1421 }
1422
1423 return modlen;
1424 }
1425 EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig);
1426
On Wed, May 25, 2022 at 3:21 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > In some cases, it is desirable to ensure that a map contains data from > authenticated sources, for example if map data are used for making security > decisions. I am guessing this comes from the discussion we had about digilim. I remember we discussed a BPF helper that could verify signatures. Why would that approach not work? > > > Such restriction is achieved by verifying the signature of map values, at > the time those values are added to the map with the bpf() system call (more > specifically, when the commands passed to bpf() are BPF_MAP_UPDATE_ELEM or > BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this case. > > Signature verification is initially done with keys in the primary and > secondary kernel keyrings, similarly to kernel modules. This allows system > owners to enforce a system-wide policy based on the keys they trust. > Support for additional keyrings could be added later, based on use case > needs. > > Signature verification is done only for those maps for which the new map > flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects map > values to be in the following format: > > +-------------------------------+---------------+-----+-----------------+ > | verified data+sig size (be32) | verified data | sig | unverified data | > +-------------------------------+---------------+-----+-----------------+ > > where sig is a module-style appended signature as generated by the > sign-file tool. The verified data+sig size (in big endian) must be > explicitly provided (it is not generated by sign-file), as it cannot be > determined in other ways (currently, the map value size is fixed). It can > be obtained from the size of the file created by sign-file. > > Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the new > function bpf_map_verify_value_sig() from bpf_map_update_value() if the flag > is set. bpf_map_verify_value_sig(), declared as global for a new helper, is > basically equivalent to mod_verify_sig(). It additionally does the marker > check, that for kernel modules is done in module_sig_check(), and the > parsing of the verified data+sig size. > > Currently, enable the usage of the flag only for the array map. Support for > more map types can be added later. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/linux/bpf.h | 7 ++++ > include/uapi/linux/bpf.h | 3 ++ > kernel/bpf/arraymap.c | 2 +- > kernel/bpf/syscall.c | 70 ++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 3 ++ > 5 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index a7080c86fa76..8f5c042e70a7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1825,6 +1825,8 @@ static inline bool unprivileged_ebpf_enabled(void) > return !sysctl_unprivileged_bpf_disabled; > } > > +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify); > + > #else /* !CONFIG_BPF_SYSCALL */ > static inline struct bpf_prog *bpf_prog_get(u32 ufd) > { > @@ -2034,6 +2036,11 @@ static inline bool unprivileged_ebpf_enabled(void) > return false; > } > > +static inline int bpf_map_verify_value_sig(const void *mod, size_t modlen, > + bool verify) > +{ > + return -EOPNOTSUPP; > +} > #endif /* CONFIG_BPF_SYSCALL */ > > void __bpf_free_used_btfs(struct bpf_prog_aux *aux, > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index f4009dbdf62d..a8e7803d2593 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1226,6 +1226,9 @@ enum { > > /* Create a map that is suitable to be an inner map with dynamic max entries */ > BPF_F_INNER_MAP = (1U << 12), > + > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ > + BPF_F_VERIFY_ELEM = (1U << 13) > }; > > /* Flags for BPF_PROG_QUERY. */ > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > index fe40d3b9458f..b430fdd0e892 100644 > --- a/kernel/bpf/arraymap.c > +++ b/kernel/bpf/arraymap.c > @@ -17,7 +17,7 @@ > > #define ARRAY_CREATE_FLAG_MASK \ > (BPF_F_NUMA_NODE | BPF_F_MMAPABLE | BPF_F_ACCESS_MASK | \ > - BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP) > + BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP | BPF_F_VERIFY_ELEM) > > static void bpf_array_free_percpu(struct bpf_array *array) > { > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 2b69306d3c6e..ca9e4a284120 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -35,6 +35,8 @@ > #include <linux/rcupdate_trace.h> > #include <linux/memcontrol.h> > #include <linux/trace_events.h> > +#include <linux/verification.h> > +#include <linux/module_signature.h> > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \ > @@ -180,6 +182,13 @@ static int bpf_map_update_value(struct bpf_map *map, struct fd f, void *key, > { > int err; > > + if (map->map_flags & BPF_F_VERIFY_ELEM) { > + err = bpf_map_verify_value_sig(value, bpf_map_value_size(map), > + true); > + if (err < 0) > + return err; > + } > + > /* Need to create a kthread, thus must support schedule */ > if (bpf_map_is_dev_bound(map)) { > return bpf_map_offload_update_elem(map, key, value, flags); > @@ -1057,6 +1066,11 @@ static int map_create(union bpf_attr *attr) > if (err) > return -EINVAL; > > + /* Allow signed data to go through update/push methods only. */ > + if ((attr->map_flags & BPF_F_VERIFY_ELEM) && > + (attr->map_flags & BPF_F_MMAPABLE)) > + return -EINVAL; > + > if (attr->btf_vmlinux_value_type_id) { > if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS || > attr->btf_key_type_id || attr->btf_value_type_id) > @@ -1353,6 +1367,62 @@ static int map_lookup_elem(union bpf_attr *attr) > return err; > } > > +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify) > +{ > + const size_t marker_len = strlen(MODULE_SIG_STRING); > + struct module_signature ms; > + size_t sig_len; > + u32 _modlen; > + int ret; > + > + /* > + * Format of mod: > + * > + * verified data+sig size (be32), verified data, sig, unverified data > + */ > + if (modlen <= sizeof(u32)) > + return -ENOENT; > + > + _modlen = be32_to_cpu(*(u32 *)(mod)); > + > + if (_modlen > modlen - sizeof(u32)) > + return -EINVAL; > + > + modlen = _modlen; > + mod += sizeof(u32); > + > + if (modlen <= marker_len) > + return -ENOENT; > + > + if (memcmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len)) > + return -ENOENT; > + > + modlen -= marker_len; > + > + if (modlen <= sizeof(ms)) > + return -EBADMSG; > + > + memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms)); > + > + ret = mod_check_sig(&ms, modlen, "bpf_map_value"); > + if (ret) > + return ret; > + > + sig_len = be32_to_cpu(ms.sig_len); > + modlen -= sig_len + sizeof(ms); > + > + if (verify) { > + ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len, > + VERIFY_USE_SECONDARY_KEYRING, > + VERIFYING_UNSPECIFIED_SIGNATURE, > + NULL, NULL); > + if (ret < 0) > + return ret; > + } > + > + return modlen; > +} > +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig); > > #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index f4009dbdf62d..a8e7803d2593 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -1226,6 +1226,9 @@ enum { > > /* Create a map that is suitable to be an inner map with dynamic max entries */ > BPF_F_INNER_MAP = (1U << 12), > + > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ > + BPF_F_VERIFY_ELEM = (1U << 13) > }; > > /* Flags for BPF_PROG_QUERY. */ > -- > 2.25.1 >
> From: KP Singh [mailto:kpsingh@kernel.org] > Sent: Friday, June 3, 2022 2:08 PM > On Wed, May 25, 2022 at 3:21 PM Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > In some cases, it is desirable to ensure that a map contains data from > > authenticated sources, for example if map data are used for making security > > decisions. > > I am guessing this comes from the discussion we had about digilim. > I remember we discussed a BPF helper that could verify signatures. > Why would that approach not work? The main reason is that signature verification can be done also for non-sleepable hooks. For example, one is fexit/array_map_update_elem. Currently the helper in patch 2 just returns the size of verified data. With an additional parameter, it could also be used as a helper for signature verification by any eBPF programs. To be honest, I like more the idea of a map flag, as it is more clear that signature verification is being done. Otherwise, we would need to infer it from the eBPF program code. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > > Such restriction is achieved by verifying the signature of map values, at > > the time those values are added to the map with the bpf() system call (more > > specifically, when the commands passed to bpf() are BPF_MAP_UPDATE_ELEM > or > > BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this case. > > > > Signature verification is initially done with keys in the primary and > > secondary kernel keyrings, similarly to kernel modules. This allows system > > owners to enforce a system-wide policy based on the keys they trust. > > Support for additional keyrings could be added later, based on use case > > needs. > > > > Signature verification is done only for those maps for which the new map > > flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects map > > values to be in the following format: > > > > +-------------------------------+---------------+-----+-----------------+ > > | verified data+sig size (be32) | verified data | sig | unverified data | > > +-------------------------------+---------------+-----+-----------------+ > > > > where sig is a module-style appended signature as generated by the > > sign-file tool. The verified data+sig size (in big endian) must be > > explicitly provided (it is not generated by sign-file), as it cannot be > > determined in other ways (currently, the map value size is fixed). It can > > be obtained from the size of the file created by sign-file. > > > > Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the > new > > function bpf_map_verify_value_sig() from bpf_map_update_value() if the flag > > is set. bpf_map_verify_value_sig(), declared as global for a new helper, is > > basically equivalent to mod_verify_sig(). It additionally does the marker > > check, that for kernel modules is done in module_sig_check(), and the > > parsing of the verified data+sig size. > > > > Currently, enable the usage of the flag only for the array map. Support for > > more map types can be added later. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > include/linux/bpf.h | 7 ++++ > > include/uapi/linux/bpf.h | 3 ++ > > kernel/bpf/arraymap.c | 2 +- > > kernel/bpf/syscall.c | 70 ++++++++++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 3 ++ > > 5 files changed, 84 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index a7080c86fa76..8f5c042e70a7 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1825,6 +1825,8 @@ static inline bool unprivileged_ebpf_enabled(void) > > return !sysctl_unprivileged_bpf_disabled; > > } > > > > +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify); > > + > > #else /* !CONFIG_BPF_SYSCALL */ > > static inline struct bpf_prog *bpf_prog_get(u32 ufd) > > { > > @@ -2034,6 +2036,11 @@ static inline bool unprivileged_ebpf_enabled(void) > > return false; > > } > > > > +static inline int bpf_map_verify_value_sig(const void *mod, size_t modlen, > > + bool verify) > > +{ > > + return -EOPNOTSUPP; > > +} > > #endif /* CONFIG_BPF_SYSCALL */ > > > > void __bpf_free_used_btfs(struct bpf_prog_aux *aux, > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index f4009dbdf62d..a8e7803d2593 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -1226,6 +1226,9 @@ enum { > > > > /* Create a map that is suitable to be an inner map with dynamic max entries > */ > > BPF_F_INNER_MAP = (1U << 12), > > + > > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ > > + BPF_F_VERIFY_ELEM = (1U << 13) > > }; > > > > /* Flags for BPF_PROG_QUERY. */ > > diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c > > index fe40d3b9458f..b430fdd0e892 100644 > > --- a/kernel/bpf/arraymap.c > > +++ b/kernel/bpf/arraymap.c > > @@ -17,7 +17,7 @@ > > > > #define ARRAY_CREATE_FLAG_MASK \ > > (BPF_F_NUMA_NODE | BPF_F_MMAPABLE | BPF_F_ACCESS_MASK | \ > > - BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP) > > + BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP | BPF_F_VERIFY_ELEM) > > > > static void bpf_array_free_percpu(struct bpf_array *array) > > { > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 2b69306d3c6e..ca9e4a284120 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -35,6 +35,8 @@ > > #include <linux/rcupdate_trace.h> > > #include <linux/memcontrol.h> > > #include <linux/trace_events.h> > > +#include <linux/verification.h> > > +#include <linux/module_signature.h> > > > > #define IS_FD_ARRAY(map) ((map)->map_type == > BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > > (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \ > > @@ -180,6 +182,13 @@ static int bpf_map_update_value(struct bpf_map > *map, struct fd f, void *key, > > { > > int err; > > > > + if (map->map_flags & BPF_F_VERIFY_ELEM) { > > + err = bpf_map_verify_value_sig(value, bpf_map_value_size(map), > > + true); > > + if (err < 0) > > + return err; > > + } > > + > > /* Need to create a kthread, thus must support schedule */ > > if (bpf_map_is_dev_bound(map)) { > > return bpf_map_offload_update_elem(map, key, value, flags); > > @@ -1057,6 +1066,11 @@ static int map_create(union bpf_attr *attr) > > if (err) > > return -EINVAL; > > > > + /* Allow signed data to go through update/push methods only. */ > > + if ((attr->map_flags & BPF_F_VERIFY_ELEM) && > > + (attr->map_flags & BPF_F_MMAPABLE)) > > + return -EINVAL; > > + > > if (attr->btf_vmlinux_value_type_id) { > > if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS || > > attr->btf_key_type_id || attr->btf_value_type_id) > > @@ -1353,6 +1367,62 @@ static int map_lookup_elem(union bpf_attr *attr) > > return err; > > } > > > > +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify) > > +{ > > + const size_t marker_len = strlen(MODULE_SIG_STRING); > > + struct module_signature ms; > > + size_t sig_len; > > + u32 _modlen; > > + int ret; > > + > > + /* > > + * Format of mod: > > + * > > + * verified data+sig size (be32), verified data, sig, unverified data > > + */ > > + if (modlen <= sizeof(u32)) > > + return -ENOENT; > > + > > + _modlen = be32_to_cpu(*(u32 *)(mod)); > > + > > + if (_modlen > modlen - sizeof(u32)) > > + return -EINVAL; > > + > > + modlen = _modlen; > > + mod += sizeof(u32); > > + > > + if (modlen <= marker_len) > > + return -ENOENT; > > + > > + if (memcmp(mod + modlen - marker_len, MODULE_SIG_STRING, > marker_len)) > > + return -ENOENT; > > + > > + modlen -= marker_len; > > + > > + if (modlen <= sizeof(ms)) > > + return -EBADMSG; > > + > > + memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms)); > > + > > + ret = mod_check_sig(&ms, modlen, "bpf_map_value"); > > + if (ret) > > + return ret; > > + > > + sig_len = be32_to_cpu(ms.sig_len); > > + modlen -= sig_len + sizeof(ms); > > + > > + if (verify) { > > + ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len, > > + VERIFY_USE_SECONDARY_KEYRING, > > + VERIFYING_UNSPECIFIED_SIGNATURE, > > + NULL, NULL); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return modlen; > > +} > > +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig); > > > > #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags > > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > index f4009dbdf62d..a8e7803d2593 100644 > > --- a/tools/include/uapi/linux/bpf.h > > +++ b/tools/include/uapi/linux/bpf.h > > @@ -1226,6 +1226,9 @@ enum { > > > > /* Create a map that is suitable to be an inner map with dynamic max entries > */ > > BPF_F_INNER_MAP = (1U << 12), > > + > > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ > > + BPF_F_VERIFY_ELEM = (1U << 13) > > }; > > > > /* Flags for BPF_PROG_QUERY. */ > > -- > > 2.25.1 > >
On Fri, Jun 3, 2022 at 3:11 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: KP Singh [mailto:kpsingh@kernel.org] > > Sent: Friday, June 3, 2022 2:08 PM > > On Wed, May 25, 2022 at 3:21 PM Roberto Sassu <roberto.sassu@huawei.com> > > wrote: > > > > > > In some cases, it is desirable to ensure that a map contains data from > > > authenticated sources, for example if map data are used for making security > > > decisions. > > > > I am guessing this comes from the discussion we had about digilim. > > I remember we discussed a BPF helper that could verify signatures. > > Why would that approach not work? > > The main reason is that signature verification can be done also > for non-sleepable hooks. For example, one is fexit/array_map_update_elem. For your use-case, why is it not possible to hook the LSM hook "bpf" i.e security_bpf and then check if there is a MAP_UPDATE_ELEM operation? > > Currently the helper in patch 2 just returns the size of verified data. > With an additional parameter, it could also be used as a helper for > signature verification by any eBPF programs. > Your bpf_map_verify_value_sig hard codes the type of signature (bpf_map_verify_value_sig as verify_pkcs7_signature) its implementation. This is not extensible. What we discussed was an extensible helper that can be used for different signature types. > To be honest, I like more the idea of a map flag, as it is more > clear that signature verification is being done. Otherwise, > we would need to infer it from the eBPF program code. > > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Li Peng, Yang Xi, Li He > > > > Such restriction is achieved by verifying the signature of map values, at > > > the time those values are added to the map with the bpf() system call (more > > > specifically, when the commands passed to bpf() are BPF_MAP_UPDATE_ELEM > > or > > > BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this case. > > > > > > Signature verification is initially done with keys in the primary and > > > secondary kernel keyrings, similarly to kernel modules. This allows system > > > owners to enforce a system-wide policy based on the keys they trust. > > > Support for additional keyrings could be added later, based on use case > > > needs. > > > > > > Signature verification is done only for those maps for which the new map > > > flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects map > > > values to be in the following format: > > > > > > +-------------------------------+---------------+-----+-----------------+ > > > | verified data+sig size (be32) | verified data | sig | unverified data | > > > +-------------------------------+---------------+-----+-----------------+ > > > > > > where sig is a module-style appended signature as generated by the > > > sign-file tool. The verified data+sig size (in big endian) must be > > > explicitly provided (it is not generated by sign-file), as it cannot be > > > determined in other ways (currently, the map value size is fixed). It can > > > be obtained from the size of the file created by sign-file. > > > > > > Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the > > new > > > function bpf_map_verify_value_sig() from bpf_map_update_value() if the flag > > > is set. bpf_map_verify_value_sig(), declared as global for a new helper, is > > > basically equivalent to mod_verify_sig(). It additionally does the marker > > > check, that for kernel modules is done in module_sig_check(), and the > > > parsing of the verified data+sig size. > > > > > > Currently, enable the usage of the flag only for the array map. Support for > > > more map types can be added later. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > --- [...] > > > + NULL, NULL); > > > + if (ret < 0) > > > + return ret; > > > + } > > > + > > > + return modlen; > > > +} > > > +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig); > > > > > > #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags > > > > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > > index f4009dbdf62d..a8e7803d2593 100644 > > > --- a/tools/include/uapi/linux/bpf.h > > > +++ b/tools/include/uapi/linux/bpf.h > > > @@ -1226,6 +1226,9 @@ enum { > > > > > > /* Create a map that is suitable to be an inner map with dynamic max entries > > */ > > > BPF_F_INNER_MAP = (1U << 12), > > > + > > > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ > > > + BPF_F_VERIFY_ELEM = (1U << 13) > > > }; > > > > > > /* Flags for BPF_PROG_QUERY. */ > > > -- > > > 2.25.1 > > >
> From: KP Singh [mailto:kpsingh@kernel.org] > Sent: Friday, June 3, 2022 5:18 PM > On Fri, Jun 3, 2022 at 3:11 PM Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > From: KP Singh [mailto:kpsingh@kernel.org] > > > Sent: Friday, June 3, 2022 2:08 PM > > > On Wed, May 25, 2022 at 3:21 PM Roberto Sassu > <roberto.sassu@huawei.com> > > > wrote: > > > > > > > > In some cases, it is desirable to ensure that a map contains data from > > > > authenticated sources, for example if map data are used for making > security > > > > decisions. > > > > > > I am guessing this comes from the discussion we had about digilim. > > > I remember we discussed a BPF helper that could verify signatures. > > > Why would that approach not work? > > > > The main reason is that signature verification can be done also > > for non-sleepable hooks. For example, one is fexit/array_map_update_elem. > > For your use-case, why is it not possible to hook the LSM hook "bpf" > i.e security_bpf and then check if there is a MAP_UPDATE_ELEM operation? It would require the following: a new helper to compare the user space fd with the address of the map in the eBPF program; copy data from user space to kernel space (verify_pkcs7_signatureI() expects kernel memory). That copy would happen twice. > > Currently the helper in patch 2 just returns the size of verified data. > > With an additional parameter, it could also be used as a helper for > > signature verification by any eBPF programs. > > > > Your bpf_map_verify_value_sig hard codes the type of signature > (bpf_map_verify_value_sig as verify_pkcs7_signature) > its implementation. This is not extensible. It is hardcoded now, but it wouldn't if there are more verification functions. For example, if 'id_type' of module_signature is set to PKEY_ID_PGP, bpf_map_verify_value_sig() would call verify_pgp_signature() (assuming that support for PGP keys and signatures is added to the kernel). Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > What we discussed was an extensible helper that can be used for > different signature types. > > > To be honest, I like more the idea of a map flag, as it is more > > clear that signature verification is being done. Otherwise, > > we would need to infer it from the eBPF program code. > > > > Thanks > > > > Roberto > > > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > > Managing Director: Li Peng, Yang Xi, Li He > > > > > > Such restriction is achieved by verifying the signature of map values, at > > > > the time those values are added to the map with the bpf() system call > (more > > > > specifically, when the commands passed to bpf() are > BPF_MAP_UPDATE_ELEM > > > or > > > > BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this > case. > > > > > > > > Signature verification is initially done with keys in the primary and > > > > secondary kernel keyrings, similarly to kernel modules. This allows system > > > > owners to enforce a system-wide policy based on the keys they trust. > > > > Support for additional keyrings could be added later, based on use case > > > > needs. > > > > > > > > Signature verification is done only for those maps for which the new map > > > > flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects > map > > > > values to be in the following format: > > > > > > > > +-------------------------------+---------------+-----+-----------------+ > > > > | verified data+sig size (be32) | verified data | sig | unverified data | > > > > +-------------------------------+---------------+-----+-----------------+ > > > > > > > > where sig is a module-style appended signature as generated by the > > > > sign-file tool. The verified data+sig size (in big endian) must be > > > > explicitly provided (it is not generated by sign-file), as it cannot be > > > > determined in other ways (currently, the map value size is fixed). It can > > > > be obtained from the size of the file created by sign-file. > > > > > > > > Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the > > > new > > > > function bpf_map_verify_value_sig() from bpf_map_update_value() if the > flag > > > > is set. bpf_map_verify_value_sig(), declared as global for a new helper, is > > > > basically equivalent to mod_verify_sig(). It additionally does the marker > > > > check, that for kernel modules is done in module_sig_check(), and the > > > > parsing of the verified data+sig size. > > > > > > > > Currently, enable the usage of the flag only for the array map. Support for > > > > more map types can be added later. > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > --- > > [...] > > > > > + NULL, NULL); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > + > > > > + return modlen; > > > > +} > > > > +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig); > > > > > > > > #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags > > > > > > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > > > > index f4009dbdf62d..a8e7803d2593 100644 > > > > --- a/tools/include/uapi/linux/bpf.h > > > > +++ b/tools/include/uapi/linux/bpf.h > > > > @@ -1226,6 +1226,9 @@ enum { > > > > > > > > /* Create a map that is suitable to be an inner map with dynamic max > entries > > > */ > > > > BPF_F_INNER_MAP = (1U << 12), > > > > + > > > > +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver > data) */ > > > > + BPF_F_VERIFY_ELEM = (1U << 13) > > > > }; > > > > > > > > /* Flags for BPF_PROG_QUERY. */ > > > > -- > > > > 2.25.1 > > > >
On Fri, Jun 3, 2022 at 5:44 PM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > > Your bpf_map_verify_value_sig hard codes the type of signature > > (bpf_map_verify_value_sig as verify_pkcs7_signature) > > its implementation. This is not extensible. > > It is hardcoded now, but it wouldn't if there are more verification > functions. For example, if 'id_type' of module_signature is set > to PKEY_ID_PGP, bpf_map_verify_value_sig() would call > verify_pgp_signature() (assuming that support for PGP keys and > signatures is added to the kernel). I agree with KP. All hard coded things are hurting extensibility. we just need a helper that calls verify_pkcs7_signature where prog will specify len, keyring, etc.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a7080c86fa76..8f5c042e70a7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1825,6 +1825,8 @@ static inline bool unprivileged_ebpf_enabled(void) return !sysctl_unprivileged_bpf_disabled; } +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify); + #else /* !CONFIG_BPF_SYSCALL */ static inline struct bpf_prog *bpf_prog_get(u32 ufd) { @@ -2034,6 +2036,11 @@ static inline bool unprivileged_ebpf_enabled(void) return false; } +static inline int bpf_map_verify_value_sig(const void *mod, size_t modlen, + bool verify) +{ + return -EOPNOTSUPP; +} #endif /* CONFIG_BPF_SYSCALL */ void __bpf_free_used_btfs(struct bpf_prog_aux *aux, diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index f4009dbdf62d..a8e7803d2593 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1226,6 +1226,9 @@ enum { /* Create a map that is suitable to be an inner map with dynamic max entries */ BPF_F_INNER_MAP = (1U << 12), + +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ + BPF_F_VERIFY_ELEM = (1U << 13) }; /* Flags for BPF_PROG_QUERY. */ diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c index fe40d3b9458f..b430fdd0e892 100644 --- a/kernel/bpf/arraymap.c +++ b/kernel/bpf/arraymap.c @@ -17,7 +17,7 @@ #define ARRAY_CREATE_FLAG_MASK \ (BPF_F_NUMA_NODE | BPF_F_MMAPABLE | BPF_F_ACCESS_MASK | \ - BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP) + BPF_F_PRESERVE_ELEMS | BPF_F_INNER_MAP | BPF_F_VERIFY_ELEM) static void bpf_array_free_percpu(struct bpf_array *array) { diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 2b69306d3c6e..ca9e4a284120 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -35,6 +35,8 @@ #include <linux/rcupdate_trace.h> #include <linux/memcontrol.h> #include <linux/trace_events.h> +#include <linux/verification.h> +#include <linux/module_signature.h> #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \ @@ -180,6 +182,13 @@ static int bpf_map_update_value(struct bpf_map *map, struct fd f, void *key, { int err; + if (map->map_flags & BPF_F_VERIFY_ELEM) { + err = bpf_map_verify_value_sig(value, bpf_map_value_size(map), + true); + if (err < 0) + return err; + } + /* Need to create a kthread, thus must support schedule */ if (bpf_map_is_dev_bound(map)) { return bpf_map_offload_update_elem(map, key, value, flags); @@ -1057,6 +1066,11 @@ static int map_create(union bpf_attr *attr) if (err) return -EINVAL; + /* Allow signed data to go through update/push methods only. */ + if ((attr->map_flags & BPF_F_VERIFY_ELEM) && + (attr->map_flags & BPF_F_MMAPABLE)) + return -EINVAL; + if (attr->btf_vmlinux_value_type_id) { if (attr->map_type != BPF_MAP_TYPE_STRUCT_OPS || attr->btf_key_type_id || attr->btf_value_type_id) @@ -1353,6 +1367,62 @@ static int map_lookup_elem(union bpf_attr *attr) return err; } +int bpf_map_verify_value_sig(const void *mod, size_t modlen, bool verify) +{ + const size_t marker_len = strlen(MODULE_SIG_STRING); + struct module_signature ms; + size_t sig_len; + u32 _modlen; + int ret; + + /* + * Format of mod: + * + * verified data+sig size (be32), verified data, sig, unverified data + */ + if (modlen <= sizeof(u32)) + return -ENOENT; + + _modlen = be32_to_cpu(*(u32 *)(mod)); + + if (_modlen > modlen - sizeof(u32)) + return -EINVAL; + + modlen = _modlen; + mod += sizeof(u32); + + if (modlen <= marker_len) + return -ENOENT; + + if (memcmp(mod + modlen - marker_len, MODULE_SIG_STRING, marker_len)) + return -ENOENT; + + modlen -= marker_len; + + if (modlen <= sizeof(ms)) + return -EBADMSG; + + memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms)); + + ret = mod_check_sig(&ms, modlen, "bpf_map_value"); + if (ret) + return ret; + + sig_len = be32_to_cpu(ms.sig_len); + modlen -= sig_len + sizeof(ms); + + if (verify) { + ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len, + VERIFY_USE_SECONDARY_KEYRING, + VERIFYING_UNSPECIFIED_SIGNATURE, + NULL, NULL); + if (ret < 0) + return ret; + } + + return modlen; +} +EXPORT_SYMBOL_GPL(bpf_map_verify_value_sig); #define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index f4009dbdf62d..a8e7803d2593 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1226,6 +1226,9 @@ enum { /* Create a map that is suitable to be an inner map with dynamic max entries */ BPF_F_INNER_MAP = (1U << 12), + +/* Verify map value (fmt: ver data+sig size(be32), ver data, sig, unver data) */ + BPF_F_VERIFY_ELEM = (1U << 13) }; /* Flags for BPF_PROG_QUERY. */
In some cases, it is desirable to ensure that a map contains data from authenticated sources, for example if map data are used for making security decisions. Such restriction is achieved by verifying the signature of map values, at the time those values are added to the map with the bpf() system call (more specifically, when the commands passed to bpf() are BPF_MAP_UPDATE_ELEM or BPF_MAP_UPDATE_BATCH). Mmappable maps are not allowed in this case. Signature verification is initially done with keys in the primary and secondary kernel keyrings, similarly to kernel modules. This allows system owners to enforce a system-wide policy based on the keys they trust. Support for additional keyrings could be added later, based on use case needs. Signature verification is done only for those maps for which the new map flag BPF_F_VERIFY_ELEM is set. When the flag is set, the kernel expects map values to be in the following format: +-------------------------------+---------------+-----+-----------------+ | verified data+sig size (be32) | verified data | sig | unverified data | +-------------------------------+---------------+-----+-----------------+ where sig is a module-style appended signature as generated by the sign-file tool. The verified data+sig size (in big endian) must be explicitly provided (it is not generated by sign-file), as it cannot be determined in other ways (currently, the map value size is fixed). It can be obtained from the size of the file created by sign-file. Introduce the new map flag BPF_F_VERIFY_ELEM, and additionally call the new function bpf_map_verify_value_sig() from bpf_map_update_value() if the flag is set. bpf_map_verify_value_sig(), declared as global for a new helper, is basically equivalent to mod_verify_sig(). It additionally does the marker check, that for kernel modules is done in module_sig_check(), and the parsing of the verified data+sig size. Currently, enable the usage of the flag only for the array map. Support for more map types can be added later. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- include/linux/bpf.h | 7 ++++ include/uapi/linux/bpf.h | 3 ++ kernel/bpf/arraymap.c | 2 +- kernel/bpf/syscall.c | 70 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 3 ++ 5 files changed, 84 insertions(+), 1 deletion(-)