Re: Mount options may be silently discarded
On Mon, Sep 28, 2020 at 5:36 PM David Laight wrote: > > From: Dmitry Kasatkin > > Sent: 28 September 2020 15:03 > > > > "copy_mount_options" function came to my eyes. > > It splits copy into 2 pieces - over page boundaries. > > I wonder what is the real reason for doing this? > > Original comment was that we need exact bytes and some user memcpy > > functions do not return correct number on page fault. > > > > But how would all other cases work? > > > > https://elixir.bootlin.com/linux/latest/source/fs/namespace.c#L3075 > > > > if (size != PAGE_SIZE) { > >if (copy_from_user(copy + size, data + size, PAGE_SIZE - size)) > > memset(copy + size, 0, PAGE_SIZE - size); > > } > > > > This looks like some options may be just discarded? > > What if it is an important security option? > > > > Why it does not return EFAULT, but just memset? > > The user doesn't supply the transfer length, the max size > is a page. > Since the copy can only start to fail on a page boundary > reading in two pieces is exactly the same as knowing the > address at which the transfer started to fail. > > Since the actual mount options can be much smaller than > a page (and usually are) zero-filling is best. > Hi David, Ok. This is now obvious that it is done for "proper" memseting... But why "we" should allow "discarding" failed part instead of failing with EFAULT as a whole? Thanks, > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 > 1PT, UK > Registration No: 1397386 (Wales) -- Thanks, Dmitry
Mount options may be silently discarded
Hi, "copy_mount_options" function came to my eyes. It splits copy into 2 pieces - over page boundaries. I wonder what is the real reason for doing this? Original comment was that we need exact bytes and some user memcpy functions do not return correct number on page fault. But how would all other cases work? https://elixir.bootlin.com/linux/latest/source/fs/namespace.c#L3075 if (size != PAGE_SIZE) { if (copy_from_user(copy + size, data + size, PAGE_SIZE - size)) memset(copy + size, 0, PAGE_SIZE - size); } This looks like some options may be just discarded? What if it is an important security option? Why it does not return EFAULT, but just memset? -- Thanks, Dmitry
Re: [PATCH -next] exec: Fix mem leak in kernel_read_file
On 13/03/2019 16:38, gre...@linuxfoundation.org wrote: On Wed, Mar 13, 2019 at 02:12:30PM +, Dmitry Kasatkin wrote: From: Sasha Levin Sent: Tuesday, March 12, 2019 1:16 AM To: Dmitry Kasatkin Cc: Al Viro; yuehaibing; linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; keesc...@chromium.org; sta...@vger.kernel.org; gre...@google.com Subject: Re: [PATCH -next] exec: Fix mem leak in kernel_read_file On Mon, Mar 11, 2019 at 04:59:14PM +, Dmitry Kasatkin wrote: From: Al Viro on behalf of Al Viro Sent: Tuesday, February 19, 2019 4:25 AM To: yuehaibing Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; Dmitry Kasatkin; keesc...@chromium.org Subject: Re: [PATCH -next] exec: Fix mem leak in kernel_read_file On Tue, Feb 19, 2019 at 10:10:38AM +0800, YueHaibing wrote: syzkaller report this: BUG: memory leak unreferenced object 0xc9000488d000 (size 9195520): comm "syz-executor.0", pid 2752, jiffies 4294787496 (age 18.757s) hex dump (first 32 bytes): ff ff ff ff ff ff ff ff a8 00 00 00 01 00 00 00 02 00 00 00 00 00 00 00 80 a1 7a c1 ff ff ff ff ..z. backtrace: [<0863775c>] __vmalloc_node mm/vmalloc.c:1795 [inline] [<0863775c>] __vmalloc_node_flags mm/vmalloc.c:1809 [inline] [<0863775c>] vmalloc+0x8c/0xb0 mm/vmalloc.c:1831 [<3f668111>] kernel_read_file+0x58f/0x7d0 fs/exec.c:924 [<2385813f>] kernel_read_file_from_fd+0x49/0x80 fs/exec.c:993 [<11953ff1>] __do_sys_finit_module+0x13b/0x2a0 kernel/module.c:3895 [<6f58491f>] do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290 [<ee78baf4>] entry_SYSCALL_64_after_hwframe+0x49/0xbe [<241f889b>] 0x It should goto 'out_free' lable to free allocated buf while kernel_read fails. Applied. This must be applied to stables as well... It's already in all relevant stable trees... I only can see in longterm 4.19. What about 4.9 and 4.14? It was in the queue already for that (you can see it on git.kernel.org), and they are now part of the -rc releases that are currently out for review. thanks, greg k-h Thanks! Dmitry
Re: [PATCH -next] exec: Fix mem leak in kernel_read_file
From: Sasha Levin Sent: Tuesday, March 12, 2019 1:16 AM To: Dmitry Kasatkin Cc: Al Viro; yuehaibing; linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; keesc...@chromium.org; sta...@vger.kernel.org; gre...@google.com Subject: Re: [PATCH -next] exec: Fix mem leak in kernel_read_file On Mon, Mar 11, 2019 at 04:59:14PM +, Dmitry Kasatkin wrote: > >From: Al Viro on behalf of Al Viro > >Sent: Tuesday, February 19, 2019 4:25 AM >To: yuehaibing >Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; Dmitry >Kasatkin; keesc...@chromium.org >Subject: Re: [PATCH -next] exec: Fix mem leak in kernel_read_file > >On Tue, Feb 19, 2019 at 10:10:38AM +0800, YueHaibing wrote: >> syzkaller report this: >> BUG: memory leak >> unreferenced object 0xc9000488d000 (size 9195520): >> comm "syz-executor.0", pid 2752, jiffies 4294787496 (age 18.757s) >> hex dump (first 32 bytes): >> ff ff ff ff ff ff ff ff a8 00 00 00 01 00 00 00 >> 02 00 00 00 00 00 00 00 80 a1 7a c1 ff ff ff ff ..z. >> backtrace: >> [<0863775c>] __vmalloc_node mm/vmalloc.c:1795 [inline] >> [<0863775c>] __vmalloc_node_flags mm/vmalloc.c:1809 [inline] >> [<0863775c>] vmalloc+0x8c/0xb0 mm/vmalloc.c:1831 >> [<3f668111>] kernel_read_file+0x58f/0x7d0 fs/exec.c:924 >> [<2385813f>] kernel_read_file_from_fd+0x49/0x80 fs/exec.c:993 >> [<11953ff1>] __do_sys_finit_module+0x13b/0x2a0 >>kernel/module.c:3895 >> [<6f58491f>] do_syscall_64+0x147/0x600 >>arch/x86/entry/common.c:290 >> [<ee78baf4>] entry_SYSCALL_64_after_hwframe+0x49/0xbe >> [<241f889b>] 0x >> >> It should goto 'out_free' lable to free allocated buf while kernel_read >> fails. > >Applied. > > >This must be applied to stables as well... > It's already in all relevant stable trees... I only can see in longterm 4.19. What about 4.9 and 4.14? Thanks, Dmitry
Re: [PATCH -next] exec: Fix mem leak in kernel_read_file
From: Al Viro on behalf of Al Viro Sent: Tuesday, February 19, 2019 4:25 AM To: yuehaibing Cc: linux-kernel@vger.kernel.org; linux-fsde...@vger.kernel.org; Dmitry Kasatkin; keesc...@chromium.org Subject: Re: [PATCH -next] exec: Fix mem leak in kernel_read_file On Tue, Feb 19, 2019 at 10:10:38AM +0800, YueHaibing wrote: > syzkaller report this: > BUG: memory leak > unreferenced object 0xc9000488d000 (size 9195520): > comm "syz-executor.0", pid 2752, jiffies 4294787496 (age 18.757s) > hex dump (first 32 bytes): > ff ff ff ff ff ff ff ff a8 00 00 00 01 00 00 00 > 02 00 00 00 00 00 00 00 80 a1 7a c1 ff ff ff ff ..z. > backtrace: > [<0863775c>] __vmalloc_node mm/vmalloc.c:1795 [inline] > [<0863775c>] __vmalloc_node_flags mm/vmalloc.c:1809 [inline] > [<0863775c>] vmalloc+0x8c/0xb0 mm/vmalloc.c:1831 > [<3f668111>] kernel_read_file+0x58f/0x7d0 fs/exec.c:924 > [<2385813f>] kernel_read_file_from_fd+0x49/0x80 fs/exec.c:993 > [<11953ff1>] __do_sys_finit_module+0x13b/0x2a0 >kernel/module.c:3895 > [<6f58491f>] do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290 > [<ee78baf4>] entry_SYSCALL_64_after_hwframe+0x49/0xbe > [<241f889b>] 0x > > It should goto 'out_free' lable to free allocated buf while kernel_read > fails. Applied. This must be applied to stables as well...
Re: [PATCH 1/7] evmtest: Regression testing Integrity Subsystem
Hi, I will have a look to patches. Thanks, Dmitry On Tue, Aug 14, 2018 at 9:34 PM James Morris wrote: > > On Tue, 14 Aug 2018, David Jacobson wrote: > > > This patchset introduces evmtest — a stand alone tool for regression > > testing IMA. > > Nice! > > I usually run the SELinux testsuite as a general sanity check of LSM > before pushing to Linus, and I'll also run this once it's merged. > > -- > James Morris > -- Thanks, Dmitry
Re: [PATCH 1/7] evmtest: Regression testing Integrity Subsystem
Hi, I will have a look to patches. Thanks, Dmitry On Tue, Aug 14, 2018 at 9:34 PM James Morris wrote: > > On Tue, 14 Aug 2018, David Jacobson wrote: > > > This patchset introduces evmtest — a stand alone tool for regression > > testing IMA. > > Nice! > > I usually run the SELinux testsuite as a general sanity check of LSM > before pushing to Linus, and I'll also run this once it's merged. > > -- > James Morris > -- Thanks, Dmitry
RE: [PATCH] lib/mpi: headers cleanup
Looks goo, you also updated comments of location of some functions. Acked-by: Dmitry Kasatkin Thanks From: Vasily Averin [v...@virtuozzo.com] Sent: Friday, June 01, 2018 7:29 PM To: Andrew Morton; linux-kernel@vger.kernel.org Cc: Dmitry Kasatkin Subject: [PATCH] lib/mpi: headers cleanup MPI headers contain definitions for huge number of non-existing functions. Most part of these functions was removed in 2012 by Dmitry Kasatkin commit 7cf4206a99d1 ("Remove unused code from MPI library") commit 9e235dcaf4f6 ("Revert "crypto: GnuPG based MPI lib - additional ...") commit bc95eeadf5c6 ("lib/mpi: removed unused functions") however headers was not updated properly. Also I deleted some unused macros. cc: Dmitry Kasatkin Signed-off-by: Vasily Averin --- include/linux/mpi.h| 61 lib/mpi/mpi-internal.h | 75 -- 2 files changed, 5 insertions(+), 131 deletions(-) diff --git a/include/linux/mpi.h b/include/linux/mpi.h index 1cc5ffb..7cd1473 100644 --- a/include/linux/mpi.h +++ b/include/linux/mpi.h @@ -53,93 +53,32 @@ struct gcry_mpi { typedef struct gcry_mpi *MPI; #define mpi_get_nlimbs(a) ((a)->nlimbs) -#define mpi_is_neg(a)((a)->sign) /*-- mpiutil.c --*/ MPI mpi_alloc(unsigned nlimbs); -MPI mpi_alloc_secure(unsigned nlimbs); -MPI mpi_alloc_like(MPI a); void mpi_free(MPI a); int mpi_resize(MPI a, unsigned nlimbs); -int mpi_copy(MPI *copy, const MPI a); -void mpi_clear(MPI a); -int mpi_set(MPI w, MPI u); -int mpi_set_ui(MPI w, ulong u); -MPI mpi_alloc_set_ui(unsigned long u); -void mpi_m_check(MPI a); -void mpi_swap(MPI a, MPI b); /*-- mpicoder.c --*/ -MPI do_encode_md(const void *sha_buffer, unsigned nbits); MPI mpi_read_raw_data(const void *xbuffer, size_t nbytes); MPI mpi_read_from_buffer(const void *buffer, unsigned *ret_nread); MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int len); -int mpi_fromstr(MPI val, const char *str); -u32 mpi_get_keyid(MPI a, u32 *keyid); void *mpi_get_buffer(MPI a, unsigned *nbytes, int *sign); int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len, unsigned *nbytes, int *sign); -void *mpi_get_secure_buffer(MPI a, unsigned *nbytes, int *sign); int mpi_write_to_sgl(MPI a, struct scatterlist *sg, unsigned nbytes, int *sign); -#define log_mpidump g10_log_mpidump - -/*-- mpi-add.c --*/ -int mpi_add_ui(MPI w, MPI u, ulong v); -int mpi_add(MPI w, MPI u, MPI v); -int mpi_addm(MPI w, MPI u, MPI v, MPI m); -int mpi_sub_ui(MPI w, MPI u, ulong v); -int mpi_sub(MPI w, MPI u, MPI v); -int mpi_subm(MPI w, MPI u, MPI v, MPI m); - -/*-- mpi-mul.c --*/ -int mpi_mul_ui(MPI w, MPI u, ulong v); -int mpi_mul_2exp(MPI w, MPI u, ulong cnt); -int mpi_mul(MPI w, MPI u, MPI v); -int mpi_mulm(MPI w, MPI u, MPI v, MPI m); - -/*-- mpi-div.c --*/ -ulong mpi_fdiv_r_ui(MPI rem, MPI dividend, ulong divisor); -int mpi_fdiv_r(MPI rem, MPI dividend, MPI divisor); -int mpi_fdiv_q(MPI quot, MPI dividend, MPI divisor); -int mpi_fdiv_qr(MPI quot, MPI rem, MPI dividend, MPI divisor); -int mpi_tdiv_r(MPI rem, MPI num, MPI den); -int mpi_tdiv_qr(MPI quot, MPI rem, MPI num, MPI den); -int mpi_tdiv_q_2exp(MPI w, MPI u, unsigned count); -int mpi_divisible_ui(const MPI dividend, ulong divisor); - -/*-- mpi-gcd.c --*/ -int mpi_gcd(MPI g, const MPI a, const MPI b); - /*-- mpi-pow.c --*/ -int mpi_pow(MPI w, MPI u, MPI v); int mpi_powm(MPI res, MPI base, MPI exp, MPI mod); -/*-- mpi-mpow.c --*/ -int mpi_mulpowm(MPI res, MPI *basearray, MPI *exparray, MPI mod); - /*-- mpi-cmp.c --*/ int mpi_cmp_ui(MPI u, ulong v); int mpi_cmp(MPI u, MPI v); -/*-- mpi-scan.c --*/ -int mpi_getbyte(MPI a, unsigned idx); -void mpi_putbyte(MPI a, unsigned idx, int value); -unsigned mpi_trailing_zeros(MPI a); - /*-- mpi-bit.c --*/ void mpi_normalize(MPI a); unsigned mpi_get_nbits(MPI a); -int mpi_test_bit(MPI a, unsigned n); -int mpi_set_bit(MPI a, unsigned n); -int mpi_set_highbit(MPI a, unsigned n); -void mpi_clear_highbit(MPI a, unsigned n); -void mpi_clear_bit(MPI a, unsigned n); -int mpi_rshift(MPI x, MPI a, unsigned n); - -/*-- mpi-inv.c --*/ -int mpi_invm(MPI x, MPI u, MPI v); /* inline functions */ diff --git a/lib/mpi/mpi-internal.h b/lib/mpi/mpi-internal.h index 7eceedd..c2d6f4e 100644 --- a/lib/mpi/mpi-internal.h +++ b/lib/mpi/mpi-internal.h @@ -65,13 +65,6 @@ typedef mpi_limb_t *mpi_ptr_t; /* pointer to a limb */ typedef int mpi_size_t;/* (must be a signed type) */ -static inline int RESIZE_IF_NEEDED(MPI a, unsigned b) -{ - if (a->alloced < b) - return mpi_resize(a, b); - return 0; -} - /* Copy N limbs from S to D. */ #define MPN_COPY(d, s, n) \ do {\ @@ -80,13 +73,6 @@ static inline int RESIZE_IF_NEEDED(MPI a, unsigned b) (d)[_i] = (s)[_i];
RE: [PATCH] lib/mpi: headers cleanup
Looks goo, you also updated comments of location of some functions. Acked-by: Dmitry Kasatkin Thanks From: Vasily Averin [v...@virtuozzo.com] Sent: Friday, June 01, 2018 7:29 PM To: Andrew Morton; linux-kernel@vger.kernel.org Cc: Dmitry Kasatkin Subject: [PATCH] lib/mpi: headers cleanup MPI headers contain definitions for huge number of non-existing functions. Most part of these functions was removed in 2012 by Dmitry Kasatkin commit 7cf4206a99d1 ("Remove unused code from MPI library") commit 9e235dcaf4f6 ("Revert "crypto: GnuPG based MPI lib - additional ...") commit bc95eeadf5c6 ("lib/mpi: removed unused functions") however headers was not updated properly. Also I deleted some unused macros. cc: Dmitry Kasatkin Signed-off-by: Vasily Averin --- include/linux/mpi.h| 61 lib/mpi/mpi-internal.h | 75 -- 2 files changed, 5 insertions(+), 131 deletions(-) diff --git a/include/linux/mpi.h b/include/linux/mpi.h index 1cc5ffb..7cd1473 100644 --- a/include/linux/mpi.h +++ b/include/linux/mpi.h @@ -53,93 +53,32 @@ struct gcry_mpi { typedef struct gcry_mpi *MPI; #define mpi_get_nlimbs(a) ((a)->nlimbs) -#define mpi_is_neg(a)((a)->sign) /*-- mpiutil.c --*/ MPI mpi_alloc(unsigned nlimbs); -MPI mpi_alloc_secure(unsigned nlimbs); -MPI mpi_alloc_like(MPI a); void mpi_free(MPI a); int mpi_resize(MPI a, unsigned nlimbs); -int mpi_copy(MPI *copy, const MPI a); -void mpi_clear(MPI a); -int mpi_set(MPI w, MPI u); -int mpi_set_ui(MPI w, ulong u); -MPI mpi_alloc_set_ui(unsigned long u); -void mpi_m_check(MPI a); -void mpi_swap(MPI a, MPI b); /*-- mpicoder.c --*/ -MPI do_encode_md(const void *sha_buffer, unsigned nbits); MPI mpi_read_raw_data(const void *xbuffer, size_t nbytes); MPI mpi_read_from_buffer(const void *buffer, unsigned *ret_nread); MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int len); -int mpi_fromstr(MPI val, const char *str); -u32 mpi_get_keyid(MPI a, u32 *keyid); void *mpi_get_buffer(MPI a, unsigned *nbytes, int *sign); int mpi_read_buffer(MPI a, uint8_t *buf, unsigned buf_len, unsigned *nbytes, int *sign); -void *mpi_get_secure_buffer(MPI a, unsigned *nbytes, int *sign); int mpi_write_to_sgl(MPI a, struct scatterlist *sg, unsigned nbytes, int *sign); -#define log_mpidump g10_log_mpidump - -/*-- mpi-add.c --*/ -int mpi_add_ui(MPI w, MPI u, ulong v); -int mpi_add(MPI w, MPI u, MPI v); -int mpi_addm(MPI w, MPI u, MPI v, MPI m); -int mpi_sub_ui(MPI w, MPI u, ulong v); -int mpi_sub(MPI w, MPI u, MPI v); -int mpi_subm(MPI w, MPI u, MPI v, MPI m); - -/*-- mpi-mul.c --*/ -int mpi_mul_ui(MPI w, MPI u, ulong v); -int mpi_mul_2exp(MPI w, MPI u, ulong cnt); -int mpi_mul(MPI w, MPI u, MPI v); -int mpi_mulm(MPI w, MPI u, MPI v, MPI m); - -/*-- mpi-div.c --*/ -ulong mpi_fdiv_r_ui(MPI rem, MPI dividend, ulong divisor); -int mpi_fdiv_r(MPI rem, MPI dividend, MPI divisor); -int mpi_fdiv_q(MPI quot, MPI dividend, MPI divisor); -int mpi_fdiv_qr(MPI quot, MPI rem, MPI dividend, MPI divisor); -int mpi_tdiv_r(MPI rem, MPI num, MPI den); -int mpi_tdiv_qr(MPI quot, MPI rem, MPI num, MPI den); -int mpi_tdiv_q_2exp(MPI w, MPI u, unsigned count); -int mpi_divisible_ui(const MPI dividend, ulong divisor); - -/*-- mpi-gcd.c --*/ -int mpi_gcd(MPI g, const MPI a, const MPI b); - /*-- mpi-pow.c --*/ -int mpi_pow(MPI w, MPI u, MPI v); int mpi_powm(MPI res, MPI base, MPI exp, MPI mod); -/*-- mpi-mpow.c --*/ -int mpi_mulpowm(MPI res, MPI *basearray, MPI *exparray, MPI mod); - /*-- mpi-cmp.c --*/ int mpi_cmp_ui(MPI u, ulong v); int mpi_cmp(MPI u, MPI v); -/*-- mpi-scan.c --*/ -int mpi_getbyte(MPI a, unsigned idx); -void mpi_putbyte(MPI a, unsigned idx, int value); -unsigned mpi_trailing_zeros(MPI a); - /*-- mpi-bit.c --*/ void mpi_normalize(MPI a); unsigned mpi_get_nbits(MPI a); -int mpi_test_bit(MPI a, unsigned n); -int mpi_set_bit(MPI a, unsigned n); -int mpi_set_highbit(MPI a, unsigned n); -void mpi_clear_highbit(MPI a, unsigned n); -void mpi_clear_bit(MPI a, unsigned n); -int mpi_rshift(MPI x, MPI a, unsigned n); - -/*-- mpi-inv.c --*/ -int mpi_invm(MPI x, MPI u, MPI v); /* inline functions */ diff --git a/lib/mpi/mpi-internal.h b/lib/mpi/mpi-internal.h index 7eceedd..c2d6f4e 100644 --- a/lib/mpi/mpi-internal.h +++ b/lib/mpi/mpi-internal.h @@ -65,13 +65,6 @@ typedef mpi_limb_t *mpi_ptr_t; /* pointer to a limb */ typedef int mpi_size_t;/* (must be a signed type) */ -static inline int RESIZE_IF_NEEDED(MPI a, unsigned b) -{ - if (a->alloced < b) - return mpi_resize(a, b); - return 0; -} - /* Copy N limbs from S to D. */ #define MPN_COPY(d, s, n) \ do {\ @@ -80,13 +73,6 @@ static inline int RESIZE_IF_NEEDED(MPI a, unsigned b) (d)[_i] = (s)[_i];
Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock
Hi, Could I ask FS maintainers to test IMA with this patch additionally and provide ack/tested. We tested but may be you have and some special testing. Thanks in advance, Dmitry On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin <dmitry.kasat...@gmail.com> wrote: > The original design was discussed 3+ years ago, but was never > completed/upstreamed. > Based on the recent discussions with Linus > https://patchwork.kernel.org/patch/9975919, I've rebased this patch. > > Before IMA appraisal was introduced, IMA was using own integrity cache > lock along with i_mutex. process_measurement and ima_file_free took > the iint->mutex first and then the i_mutex, while setxattr, chmod and > chown took the locks in reverse order. To resolve the potential deadlock, > i_mutex was moved to protect entire IMA functionality and the redundant > iint->mutex was eliminated. > > Solution was based on the assumption that filesystem code does not take > i_mutex further. But when file is opened with O_DIRECT flag, direct-io > implementation takes i_mutex and produces deadlock. Furthermore, certain > other filesystem operations, such as llseek, also take i_mutex. > > More recently some filesystems have replaced their filesystem specific > lock with the global i_rwsem to read a file. As a result, when IMA > attempts to calculate the file hash, reading the file attempts to take > the i_rwsem again. > > To resolve O_DIRECT related deadlock problem, this patch re-introduces > iint->mutex. But to eliminate the original chmod() related deadlock > problem, this patch eliminates the requirement for chmod hooks to take > the iint->mutex by introducing additional atomic iint->attr_flags to > indicate calling of the hooks. The allowed locking order is to take > the iint->mutex first and then the i_rwsem. > > Original flags were cleared in chmod(), setxattr() or removwxattr() hooks > and tested when file was closed or opened again. New atomic flags are set > or cleared in those hooks and tested to clear iint->flags on close or on open. > > Atomic flags are following: > * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) > and file attributes have changed. On file open, it causes IMA to clear > iint->flags to re-evaluate policy and perform IMA functions again. > * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and > extended attributes have changed. On file open, it causes IMA to clear > iint->flags IMA_DONE_MASK to re-appraise. > * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. > It is cleared if file policy changes and no update is needed. > * IMA_DIGSIG - indicates that file security.ima has signature and file > security.ima must not update to file has on file close. > * IMA_MUST_MEASURE - indicates the file is in the measurement policy. > > Changes in v6: > * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in > the measurement policy. It is used instead of the IMA_MEASURE (iint->flags) > to detect ToMToU violation and when iint->mutex is unlocked and behind inode > lock only (same as some other flags). Issue reported by Roberto Sassu. > > Changes in v5: > * use of inode_lock() and inode_unlock() > > Changes in v4: > * adoped to violation detection fixes > * added IMA_UPDATE_XATTR flag to require xattr update on file close > > Changes in v3: > * prevent signature removal with new locking > * rename attr_flags to atomic_flags > > Changes in v2: > * revert taking the i_mutex in integrity_inode_get() so that iint allocation > could be done with i_mutex taken > * move taking the i_mutex from appraisal code to the process_measurement() > > Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> > --- > security/integrity/iint.c | 2 + > security/integrity/ima/ima_appraise.c | 27 +++--- > security/integrity/ima/ima_main.c | 70 > --- > security/integrity/integrity.h| 18 ++--- > 4 files changed, 77 insertions(+), 40 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c84e058..d726ba23 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -155,12 +155,14 @@ static void init_once(void *foo) > memset(iint, 0, sizeof(*iint)); > iint->version = 0; > iint->flags = 0UL; > + iint->atomic_flags = 0; > iint->ima_file_status = INTEGRITY_UNKNOWN; > iint->ima_mmap_status = INTEGRITY_UNKNOWN; > iint->ima_bprm_status = INTEGRITY_UNKNOWN; > iint->ima_read_status = INTEGRITY_UNKNOWN; > iint->evm_status = INTEGR
Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock
Hi, Could I ask FS maintainers to test IMA with this patch additionally and provide ack/tested. We tested but may be you have and some special testing. Thanks in advance, Dmitry On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin wrote: > The original design was discussed 3+ years ago, but was never > completed/upstreamed. > Based on the recent discussions with Linus > https://patchwork.kernel.org/patch/9975919, I've rebased this patch. > > Before IMA appraisal was introduced, IMA was using own integrity cache > lock along with i_mutex. process_measurement and ima_file_free took > the iint->mutex first and then the i_mutex, while setxattr, chmod and > chown took the locks in reverse order. To resolve the potential deadlock, > i_mutex was moved to protect entire IMA functionality and the redundant > iint->mutex was eliminated. > > Solution was based on the assumption that filesystem code does not take > i_mutex further. But when file is opened with O_DIRECT flag, direct-io > implementation takes i_mutex and produces deadlock. Furthermore, certain > other filesystem operations, such as llseek, also take i_mutex. > > More recently some filesystems have replaced their filesystem specific > lock with the global i_rwsem to read a file. As a result, when IMA > attempts to calculate the file hash, reading the file attempts to take > the i_rwsem again. > > To resolve O_DIRECT related deadlock problem, this patch re-introduces > iint->mutex. But to eliminate the original chmod() related deadlock > problem, this patch eliminates the requirement for chmod hooks to take > the iint->mutex by introducing additional atomic iint->attr_flags to > indicate calling of the hooks. The allowed locking order is to take > the iint->mutex first and then the i_rwsem. > > Original flags were cleared in chmod(), setxattr() or removwxattr() hooks > and tested when file was closed or opened again. New atomic flags are set > or cleared in those hooks and tested to clear iint->flags on close or on open. > > Atomic flags are following: > * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) > and file attributes have changed. On file open, it causes IMA to clear > iint->flags to re-evaluate policy and perform IMA functions again. > * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and > extended attributes have changed. On file open, it causes IMA to clear > iint->flags IMA_DONE_MASK to re-appraise. > * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. > It is cleared if file policy changes and no update is needed. > * IMA_DIGSIG - indicates that file security.ima has signature and file > security.ima must not update to file has on file close. > * IMA_MUST_MEASURE - indicates the file is in the measurement policy. > > Changes in v6: > * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in > the measurement policy. It is used instead of the IMA_MEASURE (iint->flags) > to detect ToMToU violation and when iint->mutex is unlocked and behind inode > lock only (same as some other flags). Issue reported by Roberto Sassu. > > Changes in v5: > * use of inode_lock() and inode_unlock() > > Changes in v4: > * adoped to violation detection fixes > * added IMA_UPDATE_XATTR flag to require xattr update on file close > > Changes in v3: > * prevent signature removal with new locking > * rename attr_flags to atomic_flags > > Changes in v2: > * revert taking the i_mutex in integrity_inode_get() so that iint allocation > could be done with i_mutex taken > * move taking the i_mutex from appraisal code to the process_measurement() > > Signed-off-by: Dmitry Kasatkin > --- > security/integrity/iint.c | 2 + > security/integrity/ima/ima_appraise.c | 27 +++--- > security/integrity/ima/ima_main.c | 70 > --- > security/integrity/integrity.h| 18 ++--- > 4 files changed, 77 insertions(+), 40 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c84e058..d726ba23 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -155,12 +155,14 @@ static void init_once(void *foo) > memset(iint, 0, sizeof(*iint)); > iint->version = 0; > iint->flags = 0UL; > + iint->atomic_flags = 0; > iint->ima_file_status = INTEGRITY_UNKNOWN; > iint->ima_mmap_status = INTEGRITY_UNKNOWN; > iint->ima_bprm_status = INTEGRITY_UNKNOWN; > iint->ima_read_status = INTEGRITY_UNKNOWN; > iint->evm_status = INTEGRITY_UNKNOWN; > iint->measured_pcrs = 0;
[PATCHv6 1/1] ima: re-introduce own integrity cache lock
The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. * IMA_MUST_MEASURE - indicates the file is in the measurement policy. Changes in v6: * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in the measurement policy. It is used instead of the IMA_MEASURE (iint->flags) to detect ToMToU violation and when iint->mutex is unlocked and behind inode lock only (same as some other flags). Issue reported by Roberto Sassu. Changes in v5: * use of inode_lock() and inode_unlock() Changes in v4: * adoped to violation detection fixes * added IMA_UPDATE_XATTR flag to require xattr update on file close Changes in v3: * prevent signature removal with new locking * rename attr_flags to atomic_flags Changes in v2: * revert taking the i_mutex in integrity_inode_get() so that iint allocation could be done with i_mutex taken * move taking the i_mutex from appraisal code to the process_measurement() Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/iint.c | 2 + security/integrity/ima/ima_appraise.c | 27 +++--- security/integrity/ima/ima_main.c | 70 --- security/integrity/integrity.h| 18 ++--- 4 files changed, 77 insertions(+), 40 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c84e058..d726ba23 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -155,12 +155,14 @@ static void init_once(void *foo) memset(iint, 0, sizeof(*iint)); iint->version = 0; iint->flags = 0UL; + iint->atomic_flags = 0; iint->ima_file_status = INTEGRITY_UNKNOWN; iint->ima_mmap_status = INTEGRITY_UNKNOWN; iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; iint->measured_pcrs = 0; + mutex_init(>mutex); } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9a54c77..3fc96dbd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_FAIL; break; } + clear_bit(IMA_DIGSIG, >atomic_flags); if (xattr_len - sizeof(xattr_value->type) - hash_start >= iint->ima_hash->
[PATCHv6 1/1] ima: re-introduce own integrity cache lock
The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. * IMA_MUST_MEASURE - indicates the file is in the measurement policy. Changes in v6: * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in the measurement policy. It is used instead of the IMA_MEASURE (iint->flags) to detect ToMToU violation and when iint->mutex is unlocked and behind inode lock only (same as some other flags). Issue reported by Roberto Sassu. Changes in v5: * use of inode_lock() and inode_unlock() Changes in v4: * adoped to violation detection fixes * added IMA_UPDATE_XATTR flag to require xattr update on file close Changes in v3: * prevent signature removal with new locking * rename attr_flags to atomic_flags Changes in v2: * revert taking the i_mutex in integrity_inode_get() so that iint allocation could be done with i_mutex taken * move taking the i_mutex from appraisal code to the process_measurement() Signed-off-by: Dmitry Kasatkin --- security/integrity/iint.c | 2 + security/integrity/ima/ima_appraise.c | 27 +++--- security/integrity/ima/ima_main.c | 70 --- security/integrity/integrity.h| 18 ++--- 4 files changed, 77 insertions(+), 40 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c84e058..d726ba23 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -155,12 +155,14 @@ static void init_once(void *foo) memset(iint, 0, sizeof(*iint)); iint->version = 0; iint->flags = 0UL; + iint->atomic_flags = 0; iint->ima_file_status = INTEGRITY_UNKNOWN; iint->ima_mmap_status = INTEGRITY_UNKNOWN; iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; iint->measured_pcrs = 0; + mutex_init(>mutex); } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9a54c77..3fc96dbd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_FAIL; break; } + clear_bit(IMA_DIGSIG, >atomic_flags); if (xattr_len - sizeof(xattr_value->type) - hash_start >= iint->ima_hash->length)
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 17:40, Dmitry Kasatkin wrote: On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, Dmitry Kasatkin wrote: The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry It seems you are right... That violation patch came in between locking patch was there. I do not remember why I have rebased it like it looks now. But it seems that violation checking needs to be moved under iint->mutex locking. Hmm. but why I have not done it like that 3 years ago :) I will think how to update it. Thanks for catching it up. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 17:40, Dmitry Kasatkin wrote: On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, Dmitry Kasatkin wrote: The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry It seems you are right... That violation patch came in between locking patch was there. I do not remember why I have rebased it like it looks now. But it seems that violation checking needs to be moved under iint->mutex locking. Hmm. but why I have not done it like that 3 years ago :) I will think how to update it. Thanks for catching it up. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, Dmitry Kasatkin wrote: The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 1/1] ima: re-introduce own integrity cache lock
On 04/12/17 15:42, Roberto Sassu wrote: On 12/4/2017 1:06 PM, Mimi Zohar wrote: Hi Dmitry, On Fri, 2017-12-01 at 20:40 +0200, Dmitry Kasatkin wrote: The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Nice! I've been testing with this patch and all seems good. Before queueing this patch to be upstreamed, I'd appreciate if others tested using it as well. It applies cleanly to the next-queued branch. If the inode lock is released before the IMA_MEASURE flag is set, the ToMToU violation will not be detected when a writer accesses the same inode. This issue was fixed with commit f7a859ff7395c. Roberto Hi Roberto, I will check the commit. Dmitry A subsequent patch will remove the O_DIRECT check in ima_calc_file_hash(). Fixes: Commit 6552321831dc "xfs: remove i_iolock and use i_rwsem in the VFS inode instead" thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv5 1/1] ima: re-introduce own integrity cache lock
The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Changes in v5: * use of inode_lock() and inode_unlock() Changes in v4: * adoped to violation detection fixes * added IMA_UPDATE_XATTR flag to require xattr update on file close Changes in v3: * prevent signature removal with new locking * rename attr_flags to atomic_flags Changes in v2: * revert taking the i_mutex in integrity_inode_get() so that iint allocation could be done with i_mutex taken * move taking the i_mutex from appraisal code to the process_measurement() Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/iint.c | 2 ++ security/integrity/ima/ima_appraise.c | 27 --- security/integrity/ima/ima_main.c | 65 --- security/integrity/integrity.h| 17 ++--- 4 files changed, 72 insertions(+), 39 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c84e058..d726ba23 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -155,12 +155,14 @@ static void init_once(void *foo) memset(iint, 0, sizeof(*iint)); iint->version = 0; iint->flags = 0UL; + iint->atomic_flags = 0; iint->ima_file_status = INTEGRITY_UNKNOWN; iint->ima_mmap_status = INTEGRITY_UNKNOWN; iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; iint->measured_pcrs = 0; + mutex_init(>mutex); } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9a54c77..3fc96dbd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_FAIL; break; } + clear_bit(IMA_DIGSIG, >atomic_flags); if (xattr_len - sizeof(xattr_value->type) - hash_start >= iint->ima_hash->length) /* xattr length may be longer. md5 hash in previous @@ -269,7 +270,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; break; case EVM_IMA_XATTR_DIGSIG: - iint->flags |= IMA_DIGSIG; + set_bit(IMA_DIGSIG, >atomic_flags); rc = integrity_digsig_ve
[PATCHv5 1/1] ima: re-introduce own integrity cache lock
The original design was discussed 3+ years ago, but was never completed/upstreamed. Based on the recent discussions with Linus https://patchwork.kernel.org/patch/9975919, I've rebased this patch. Before IMA appraisal was introduced, IMA was using own integrity cache lock along with i_mutex. process_measurement and ima_file_free took the iint->mutex first and then the i_mutex, while setxattr, chmod and chown took the locks in reverse order. To resolve the potential deadlock, i_mutex was moved to protect entire IMA functionality and the redundant iint->mutex was eliminated. Solution was based on the assumption that filesystem code does not take i_mutex further. But when file is opened with O_DIRECT flag, direct-io implementation takes i_mutex and produces deadlock. Furthermore, certain other filesystem operations, such as llseek, also take i_mutex. More recently some filesystems have replaced their filesystem specific lock with the global i_rwsem to read a file. As a result, when IMA attempts to calculate the file hash, reading the file attempts to take the i_rwsem again. To resolve O_DIRECT related deadlock problem, this patch re-introduces iint->mutex. But to eliminate the original chmod() related deadlock problem, this patch eliminates the requirement for chmod hooks to take the iint->mutex by introducing additional atomic iint->attr_flags to indicate calling of the hooks. The allowed locking order is to take the iint->mutex first and then the i_rwsem. Original flags were cleared in chmod(), setxattr() or removwxattr() hooks and tested when file was closed or opened again. New atomic flags are set or cleared in those hooks and tested to clear iint->flags on close or on open. Atomic flags are following: * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) and file attributes have changed. On file open, it causes IMA to clear iint->flags to re-evaluate policy and perform IMA functions again. * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and extended attributes have changed. On file open, it causes IMA to clear iint->flags IMA_DONE_MASK to re-appraise. * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. It is cleared if file policy changes and no update is needed. * IMA_DIGSIG - indicates that file security.ima has signature and file security.ima must not update to file has on file close. Changes in v5: * use of inode_lock() and inode_unlock() Changes in v4: * adoped to violation detection fixes * added IMA_UPDATE_XATTR flag to require xattr update on file close Changes in v3: * prevent signature removal with new locking * rename attr_flags to atomic_flags Changes in v2: * revert taking the i_mutex in integrity_inode_get() so that iint allocation could be done with i_mutex taken * move taking the i_mutex from appraisal code to the process_measurement() Signed-off-by: Dmitry Kasatkin --- security/integrity/iint.c | 2 ++ security/integrity/ima/ima_appraise.c | 27 --- security/integrity/ima/ima_main.c | 65 --- security/integrity/integrity.h| 17 ++--- 4 files changed, 72 insertions(+), 39 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c84e058..d726ba23 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -155,12 +155,14 @@ static void init_once(void *foo) memset(iint, 0, sizeof(*iint)); iint->version = 0; iint->flags = 0UL; + iint->atomic_flags = 0; iint->ima_file_status = INTEGRITY_UNKNOWN; iint->ima_mmap_status = INTEGRITY_UNKNOWN; iint->ima_bprm_status = INTEGRITY_UNKNOWN; iint->ima_read_status = INTEGRITY_UNKNOWN; iint->evm_status = INTEGRITY_UNKNOWN; iint->measured_pcrs = 0; + mutex_init(>mutex); } static int __init integrity_iintcache_init(void) diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 9a54c77..3fc96dbd 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_FAIL; break; } + clear_bit(IMA_DIGSIG, >atomic_flags); if (xattr_len - sizeof(xattr_value->type) - hash_start >= iint->ima_hash->length) /* xattr length may be longer. md5 hash in previous @@ -269,7 +270,7 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; break; case EVM_IMA_XATTR_DIGSIG: - iint->flags |= IMA_DIGSIG; + set_bit(IMA_DIGSIG, >atomic_flags); rc = integrity_digsig_verify(INTEGRITY_KEYRING_I
Re: [PATCHC v7 00/10] ima: carry the measurement list across kexec
On Thu, Nov 10, 2016 at 4:56 PM, Mimi Zoharwrote: > [Posting with abbreviated Cc list.] > > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and then restored on the subsequent > boot, possibly of a different architecture. > > The existing securityfs binary_runtime_measurements file conveniently > provides a serialized format of the IMA measurement list. This patch > set serializes the measurement list in this format and restores it. > > Up to now, the binary_runtime_measurements was defined as architecture > native format. The assumption being that userspace could and would > handle any architecture conversions. With the ability of carrying the > measurement list across kexec, possibly from one architecture to a > different one, the per boot architecture information is lost and with it > the ability of recalculating the template digest hash. To resolve this > problem, without breaking the existing ABI, this patch set introduces > the boot command line option "ima_canonical_fmt", which is arbitrarily > defined as little endian. > > The need for this boot command line option will be limited to the > existing version 1 format of the binary_runtime_measurements. > Subsequent formats will be defined as canonical format (eg. TPM 2.0 > support for larger digests). > > A simplified method of Thiago Bauermann's "kexec buffer handover" patch > series for carrying the IMA measurement list across kexec is included > in this patch set. The simplified method requires all file measurements > be taken prior to executing the kexec load, as subsequent measurements > will not be carried across the kexec and restored. > > These patches can also be found in the next-kexec-restore branch of: > git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git > > Changelog v7: > - Updated to reflect Dmitry Kasatkin's patch review > - Rebased on top of "next-fixes" branch > > Andreas Steffen (1): > ima: platform-independent hash value > > Mimi Zohar (7): > ima: on soft reboot, restore the measurement list > ima: permit duplicate measurement list entries > ima: maintain memory size needed for serializing the measurement list > ima: on soft reboot, save the measurement list > ima: store the builtin/custom template definitions in a list > ima: support restoring multiple template formats > ima: define a canonical binary_runtime_measurements list format > > Thiago Jung Bauermann (2): > powerpc: ima: Get the kexec buffer passed by the previous kernel > powerpc: ima: Send the kexec buffer to the next kernel > > Documentation/kernel-parameters.txt | 4 + > arch/Kconfig| 3 + > arch/powerpc/Kconfig| 1 + > arch/powerpc/include/asm/ima.h | 29 +++ > arch/powerpc/include/asm/kexec.h| 15 +- > arch/powerpc/kernel/Makefile| 4 + > arch/powerpc/kernel/ima_kexec.c | 223 + > arch/powerpc/kernel/kexec_elf_64.c | 2 +- > arch/powerpc/kernel/machine_kexec_file_64.c | 15 +- > include/linux/ima.h | 12 ++ > kernel/kexec_file.c | 4 + > security/integrity/ima/Kconfig | 12 ++ > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h| 31 +++ > security/integrity/ima/ima_crypto.c | 6 +- > security/integrity/ima/ima_fs.c | 30 ++- > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c | 168 > security/integrity/ima/ima_main.c | 1 + > security/integrity/ima/ima_queue.c | 77 +++- > security/integrity/ima/ima_template.c | 297 > ++-- > security/integrity/ima/ima_template_lib.c | 7 +- > 22 files changed, 906 insertions(+), 38 deletions(-) > create mode 100644 arch/powerpc/include/asm/ima.h > create mode 100644 arch/powerpc/kernel/ima_kexec.c > create mode 100644 security/integrity/ima/ima_kexec.c > > -- > 2.1.0 > Hi, Looks good to me. -- Thanks, Dmitry
Re: [PATCHC v7 00/10] ima: carry the measurement list across kexec
On Thu, Nov 10, 2016 at 4:56 PM, Mimi Zohar wrote: > [Posting with abbreviated Cc list.] > > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and then restored on the subsequent > boot, possibly of a different architecture. > > The existing securityfs binary_runtime_measurements file conveniently > provides a serialized format of the IMA measurement list. This patch > set serializes the measurement list in this format and restores it. > > Up to now, the binary_runtime_measurements was defined as architecture > native format. The assumption being that userspace could and would > handle any architecture conversions. With the ability of carrying the > measurement list across kexec, possibly from one architecture to a > different one, the per boot architecture information is lost and with it > the ability of recalculating the template digest hash. To resolve this > problem, without breaking the existing ABI, this patch set introduces > the boot command line option "ima_canonical_fmt", which is arbitrarily > defined as little endian. > > The need for this boot command line option will be limited to the > existing version 1 format of the binary_runtime_measurements. > Subsequent formats will be defined as canonical format (eg. TPM 2.0 > support for larger digests). > > A simplified method of Thiago Bauermann's "kexec buffer handover" patch > series for carrying the IMA measurement list across kexec is included > in this patch set. The simplified method requires all file measurements > be taken prior to executing the kexec load, as subsequent measurements > will not be carried across the kexec and restored. > > These patches can also be found in the next-kexec-restore branch of: > git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git > > Changelog v7: > - Updated to reflect Dmitry Kasatkin's patch review > - Rebased on top of "next-fixes" branch > > Andreas Steffen (1): > ima: platform-independent hash value > > Mimi Zohar (7): > ima: on soft reboot, restore the measurement list > ima: permit duplicate measurement list entries > ima: maintain memory size needed for serializing the measurement list > ima: on soft reboot, save the measurement list > ima: store the builtin/custom template definitions in a list > ima: support restoring multiple template formats > ima: define a canonical binary_runtime_measurements list format > > Thiago Jung Bauermann (2): > powerpc: ima: Get the kexec buffer passed by the previous kernel > powerpc: ima: Send the kexec buffer to the next kernel > > Documentation/kernel-parameters.txt | 4 + > arch/Kconfig| 3 + > arch/powerpc/Kconfig| 1 + > arch/powerpc/include/asm/ima.h | 29 +++ > arch/powerpc/include/asm/kexec.h| 15 +- > arch/powerpc/kernel/Makefile| 4 + > arch/powerpc/kernel/ima_kexec.c | 223 + > arch/powerpc/kernel/kexec_elf_64.c | 2 +- > arch/powerpc/kernel/machine_kexec_file_64.c | 15 +- > include/linux/ima.h | 12 ++ > kernel/kexec_file.c | 4 + > security/integrity/ima/Kconfig | 12 ++ > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h| 31 +++ > security/integrity/ima/ima_crypto.c | 6 +- > security/integrity/ima/ima_fs.c | 30 ++- > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c | 168 > security/integrity/ima/ima_main.c | 1 + > security/integrity/ima/ima_queue.c | 77 +++- > security/integrity/ima/ima_template.c | 297 > ++-- > security/integrity/ima/ima_template_lib.c | 7 +- > 22 files changed, 906 insertions(+), 38 deletions(-) > create mode 100644 arch/powerpc/include/asm/ima.h > create mode 100644 arch/powerpc/kernel/ima_kexec.c > create mode 100644 security/integrity/ima/ima_kexec.c > > -- > 2.1.0 > Hi, Looks good to me. -- Thanks, Dmitry
Re: [PATCHC v7 02/10] ima: on soft reboot, restore the measurement list
On Thu, Nov 10, 2016 at 4:56 PM, Mimi Zoharwrote: > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and restored on boot. This patch > restores the measurement list. > > Changelog v7: > - add and fix missing buffer length checking (reported by D. Kasatkin) > - clean up ima_restore_measurement_list() a bit > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago) > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago) > - remove unnecessary includes from ima_kexec.c (Thiago) > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King) > > Changelog v2: > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman) > - defined missing ima_load_kexec_buffer() stub function > > Changelog v1: > - call ima_load_kexec_buffer() (Thiago) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 21 > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c| 44 + > security/integrity/ima/ima_queue.c| 10 ++ > security/integrity/ima/ima_template.c | 177 > ++ > 6 files changed, 255 insertions(+) > create mode 100644 security/integrity/ima/ima_kexec.c > > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 9aeaedad1e2b..29f198bde02b 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ > ima_policy.o ima_template.o ima_template_lib.o > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index db25f54a04fe..51dc8d57d64d 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -28,6 +28,10 @@ > > #include "../integrity.h" > > +#ifdef CONFIG_HAVE_IMA_KEXEC > +#include > +#endif > + > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; > @@ -102,6 +106,21 @@ struct ima_queue_entry { > }; > extern struct list_head ima_measurements; /* list of all measurements */ > > +/* Some details preceding the binary serialized measurement list */ > +struct ima_kexec_hdr { > + u16 version; > + u16 _reserved0; > + u32 _reserved1; > + u64 buffer_size; > + u64 count; > +}; > + > +#ifdef CONFIG_HAVE_IMA_KEXEC > +void ima_load_kexec_buffer(void); > +#else > +static inline void ima_load_kexec_buffer(void) {} > +#endif /* CONFIG_HAVE_IMA_KEXEC */ > + > /* Internal IMA function definitions */ > int ima_init(void); > int ima_fs_init(void); > @@ -122,6 +141,8 @@ int ima_init_crypto(void); > void ima_putc(struct seq_file *m, void *data, int datalen); > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); > struct ima_template_desc *ima_template_desc_current(void); > +int ima_restore_measurement_entry(struct ima_template_entry *entry); > +int ima_restore_measurement_list(loff_t bufsize, void *buf); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > index 2ac1f41db5c0..2967d497a665 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -129,6 +129,8 @@ int __init ima_init(void) > if (rc != 0) > return rc; > > + ima_load_kexec_buffer(); > + > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry > */ > if (rc != 0) > return rc; > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > new file mode 100644 > index ..36afd0fe9747 > --- /dev/null > +++ b/security/integrity/ima/ima_kexec.c > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2016 IBM Corporation > + * > + * Authors: > + * Thiago Jung Bauermann > + * Mimi Zohar > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include "ima.h" > + > +/* > + * Restore the measurement list from the previous kernel. > + */ > +void ima_load_kexec_buffer(void) > +{ > + void *kexec_buffer = NULL;
Re: [PATCHC v7 02/10] ima: on soft reboot, restore the measurement list
On Thu, Nov 10, 2016 at 4:56 PM, Mimi Zohar wrote: > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and restored on boot. This patch > restores the measurement list. > > Changelog v7: > - add and fix missing buffer length checking (reported by D. Kasatkin) > - clean up ima_restore_measurement_list() a bit > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago) > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago) > - remove unnecessary includes from ima_kexec.c (Thiago) > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King) > > Changelog v2: > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman) > - defined missing ima_load_kexec_buffer() stub function > > Changelog v1: > - call ima_load_kexec_buffer() (Thiago) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 21 > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c| 44 + > security/integrity/ima/ima_queue.c| 10 ++ > security/integrity/ima/ima_template.c | 177 > ++ > 6 files changed, 255 insertions(+) > create mode 100644 security/integrity/ima/ima_kexec.c > > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 9aeaedad1e2b..29f198bde02b 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ > ima_policy.o ima_template.o ima_template_lib.o > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index db25f54a04fe..51dc8d57d64d 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -28,6 +28,10 @@ > > #include "../integrity.h" > > +#ifdef CONFIG_HAVE_IMA_KEXEC > +#include > +#endif > + > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; > @@ -102,6 +106,21 @@ struct ima_queue_entry { > }; > extern struct list_head ima_measurements; /* list of all measurements */ > > +/* Some details preceding the binary serialized measurement list */ > +struct ima_kexec_hdr { > + u16 version; > + u16 _reserved0; > + u32 _reserved1; > + u64 buffer_size; > + u64 count; > +}; > + > +#ifdef CONFIG_HAVE_IMA_KEXEC > +void ima_load_kexec_buffer(void); > +#else > +static inline void ima_load_kexec_buffer(void) {} > +#endif /* CONFIG_HAVE_IMA_KEXEC */ > + > /* Internal IMA function definitions */ > int ima_init(void); > int ima_fs_init(void); > @@ -122,6 +141,8 @@ int ima_init_crypto(void); > void ima_putc(struct seq_file *m, void *data, int datalen); > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); > struct ima_template_desc *ima_template_desc_current(void); > +int ima_restore_measurement_entry(struct ima_template_entry *entry); > +int ima_restore_measurement_list(loff_t bufsize, void *buf); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > index 2ac1f41db5c0..2967d497a665 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -129,6 +129,8 @@ int __init ima_init(void) > if (rc != 0) > return rc; > > + ima_load_kexec_buffer(); > + > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry > */ > if (rc != 0) > return rc; > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > new file mode 100644 > index ..36afd0fe9747 > --- /dev/null > +++ b/security/integrity/ima/ima_kexec.c > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2016 IBM Corporation > + * > + * Authors: > + * Thiago Jung Bauermann > + * Mimi Zohar > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include "ima.h" > + > +/* > + * Restore the measurement list from the previous kernel. > + */ > +void ima_load_kexec_buffer(void) > +{ > + void *kexec_buffer = NULL; > + size_t kexec_buffer_size = 0; > + int rc; > + > + rc = ima_get_kexec_buffer(_buffer,
Re: [Linux-ima-devel] [PATCH v6 07/10] ima: store the builtin/custom template definitions in a list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermannwrote: > From: Mimi Zohar > > The builtin and single custom templates are currently stored in an > array. In preparation for being able to restore a measurement list > containing multiple builtin/custom templates, this patch stores the > builtin and custom templates as a linked list. This will permit > defining more than one custom template per boot. > > Changelog v4: > - fix "spinlock bad magic" BUG - reported by Dmitry Vyukov > > Changelog v3: > - initialize template format list in ima_template_desc_current(), as it > might be called during __setup before normal initialization. (kernel > test robot) > - remove __init annotation of ima_init_template_list() > > Changelog v2: > - fix lookup_template_desc() preemption imbalance (kernel test robot) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/ima.h | 2 ++ > security/integrity/ima/ima_main.c | 1 + > security/integrity/ima/ima_template.c | 52 > +++ > 3 files changed, 44 insertions(+), 11 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 139dec67dcbf..6b0540ad189f 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -85,6 +85,7 @@ struct ima_template_field { > > /* IMA template descriptor definition */ > struct ima_template_desc { > + struct list_head list; > char *name; > char *fmt; > int num_fields; > @@ -146,6 +147,7 @@ int ima_restore_measurement_list(loff_t bufsize, void > *buf); > int ima_measurements_show(struct seq_file *m, void *v); > unsigned long ima_get_binary_runtime_size(void); > int ima_init_template(void); > +void ima_init_template_list(void); > > /* > * used to protect h_table and sha_table > diff --git a/security/integrity/ima/ima_main.c > b/security/integrity/ima/ima_main.c > index 423d111b3b94..50818c60538b 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -418,6 +418,7 @@ static int __init init_ima(void) > { > int error; > > + ima_init_template_list(); > hash_setup(CONFIG_IMA_DEFAULT_HASH); > error = ima_init(); > if (!error) { > diff --git a/security/integrity/ima/ima_template.c > b/security/integrity/ima/ima_template.c > index 37f972cb05fe..c0d808c20c40 100644 > --- a/security/integrity/ima/ima_template.c > +++ b/security/integrity/ima/ima_template.c > @@ -15,16 +15,20 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include > #include "ima.h" > #include "ima_template_lib.h" > > -static struct ima_template_desc defined_templates[] = { > +static struct ima_template_desc builtin_templates[] = { > {.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT}, > {.name = "ima-ng", .fmt = "d-ng|n-ng"}, > {.name = "ima-sig", .fmt = "d-ng|n-ng|sig"}, > {.name = "", .fmt = ""},/* placeholder for a custom format */ > }; > > +static LIST_HEAD(defined_templates); > +static DEFINE_SPINLOCK(template_list); > + > static struct ima_template_field supported_fields[] = { > {.field_id = "d", .field_init = ima_eventdigest_init, > .field_show = ima_show_template_digest}, > @@ -53,6 +57,8 @@ static int __init ima_template_setup(char *str) > if (ima_template) > return 1; > > + ima_init_template_list(); > + > /* > * Verify that a template with the supplied name exists. > * If not, use CONFIG_IMA_DEFAULT_TEMPLATE. > @@ -81,7 +87,7 @@ __setup("ima_template=", ima_template_setup); > > static int __init ima_template_fmt_setup(char *str) > { > - int num_templates = ARRAY_SIZE(defined_templates); > + int num_templates = ARRAY_SIZE(builtin_templates); > > if (ima_template) > return 1; > @@ -92,22 +98,28 @@ static int __init ima_template_fmt_setup(char *str) > return 1; > } > > - defined_templates[num_templates - 1].fmt = str; > - ima_template = defined_templates + num_templates - 1; > + builtin_templates[num_templates - 1].fmt = str; > + ima_template = builtin_templates + num_templates - 1; > + > return 1; > } > __setup("ima_template_fmt=", ima_template_fmt_setup); > > static struct ima_template_desc *lookup_template_desc(const char *name) > { > - int i; > + struct ima_template_desc *template_desc; > + int found = 0; > > - for (i = 0; i < ARRAY_SIZE(defined_templates); i++) { > - if (strcmp(defined_templates[i].name, name) == 0) > - return defined_templates + i; > + rcu_read_lock(); > + list_for_each_entry_rcu(template_desc, _templates, list) { > + if ((strcmp(template_desc->name, name) == 0) || > + (strcmp(template_desc->fmt, name) ==
Re: [Linux-ima-devel] [PATCH v6 07/10] ima: store the builtin/custom template definitions in a list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann wrote: > From: Mimi Zohar > > The builtin and single custom templates are currently stored in an > array. In preparation for being able to restore a measurement list > containing multiple builtin/custom templates, this patch stores the > builtin and custom templates as a linked list. This will permit > defining more than one custom template per boot. > > Changelog v4: > - fix "spinlock bad magic" BUG - reported by Dmitry Vyukov > > Changelog v3: > - initialize template format list in ima_template_desc_current(), as it > might be called during __setup before normal initialization. (kernel > test robot) > - remove __init annotation of ima_init_template_list() > > Changelog v2: > - fix lookup_template_desc() preemption imbalance (kernel test robot) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/ima.h | 2 ++ > security/integrity/ima/ima_main.c | 1 + > security/integrity/ima/ima_template.c | 52 > +++ > 3 files changed, 44 insertions(+), 11 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 139dec67dcbf..6b0540ad189f 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -85,6 +85,7 @@ struct ima_template_field { > > /* IMA template descriptor definition */ > struct ima_template_desc { > + struct list_head list; > char *name; > char *fmt; > int num_fields; > @@ -146,6 +147,7 @@ int ima_restore_measurement_list(loff_t bufsize, void > *buf); > int ima_measurements_show(struct seq_file *m, void *v); > unsigned long ima_get_binary_runtime_size(void); > int ima_init_template(void); > +void ima_init_template_list(void); > > /* > * used to protect h_table and sha_table > diff --git a/security/integrity/ima/ima_main.c > b/security/integrity/ima/ima_main.c > index 423d111b3b94..50818c60538b 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -418,6 +418,7 @@ static int __init init_ima(void) > { > int error; > > + ima_init_template_list(); > hash_setup(CONFIG_IMA_DEFAULT_HASH); > error = ima_init(); > if (!error) { > diff --git a/security/integrity/ima/ima_template.c > b/security/integrity/ima/ima_template.c > index 37f972cb05fe..c0d808c20c40 100644 > --- a/security/integrity/ima/ima_template.c > +++ b/security/integrity/ima/ima_template.c > @@ -15,16 +15,20 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include > #include "ima.h" > #include "ima_template_lib.h" > > -static struct ima_template_desc defined_templates[] = { > +static struct ima_template_desc builtin_templates[] = { > {.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT}, > {.name = "ima-ng", .fmt = "d-ng|n-ng"}, > {.name = "ima-sig", .fmt = "d-ng|n-ng|sig"}, > {.name = "", .fmt = ""},/* placeholder for a custom format */ > }; > > +static LIST_HEAD(defined_templates); > +static DEFINE_SPINLOCK(template_list); > + > static struct ima_template_field supported_fields[] = { > {.field_id = "d", .field_init = ima_eventdigest_init, > .field_show = ima_show_template_digest}, > @@ -53,6 +57,8 @@ static int __init ima_template_setup(char *str) > if (ima_template) > return 1; > > + ima_init_template_list(); > + > /* > * Verify that a template with the supplied name exists. > * If not, use CONFIG_IMA_DEFAULT_TEMPLATE. > @@ -81,7 +87,7 @@ __setup("ima_template=", ima_template_setup); > > static int __init ima_template_fmt_setup(char *str) > { > - int num_templates = ARRAY_SIZE(defined_templates); > + int num_templates = ARRAY_SIZE(builtin_templates); > > if (ima_template) > return 1; > @@ -92,22 +98,28 @@ static int __init ima_template_fmt_setup(char *str) > return 1; > } > > - defined_templates[num_templates - 1].fmt = str; > - ima_template = defined_templates + num_templates - 1; > + builtin_templates[num_templates - 1].fmt = str; > + ima_template = builtin_templates + num_templates - 1; > + > return 1; > } > __setup("ima_template_fmt=", ima_template_fmt_setup); > > static struct ima_template_desc *lookup_template_desc(const char *name) > { > - int i; > + struct ima_template_desc *template_desc; > + int found = 0; > > - for (i = 0; i < ARRAY_SIZE(defined_templates); i++) { > - if (strcmp(defined_templates[i].name, name) == 0) > - return defined_templates + i; > + rcu_read_lock(); > + list_for_each_entry_rcu(template_desc, _templates, list) { > + if ((strcmp(template_desc->name, name) == 0) || > + (strcmp(template_desc->fmt, name) == 0)) { > + found = 1; > + break; > +
Re: [Linux-ima-devel] [PATCH v6 04/10] ima: maintain memory size needed for serializing the measurement list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermannwrote: > From: Mimi Zohar > > In preparation for serializing the binary_runtime_measurements, this patch > maintains the amount of memory required. > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > > Changelog v3: > - include the ima_kexec_hdr size in the binary_runtime_measurement size. > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Kconfig | 12 + > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_queue.c | 53 > -- > 3 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 5487827fa86c..370eb2f4dd37 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -27,6 +27,18 @@ config IMA > to learn more about IMA. > If unsure, say N. > > +config IMA_KEXEC > + bool "Enable carrying the IMA measurement list across a soft boot" > + depends on IMA && TCG_TPM && HAVE_IMA_KEXEC > + default n > + help > + TPM PCRs are only reset on a hard reboot. In order to validate > + a TPM's quote after a soft boot, the IMA measurement list of the > + running kernel must be saved and restored on boot. > + > + Depending on the IMA policy, the measurement list can grow to > + be very large. > + > config IMA_MEASURE_PCR_IDX > int > depends on IMA > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 51dc8d57d64d..ea1dcc452911 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -143,6 +143,7 @@ void ima_print_digest(struct seq_file *m, u8 *digest, u32 > size); > struct ima_template_desc *ima_template_desc_current(void); > int ima_restore_measurement_entry(struct ima_template_entry *entry); > int ima_restore_measurement_list(loff_t bufsize, void *buf); > +unsigned long ima_get_binary_runtime_size(void); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_queue.c > b/security/integrity/ima/ima_queue.c > index 12d1b040bca9..3a3cc2a45645 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -29,6 +29,11 @@ > #define AUDIT_CAUSE_LEN_MAX 32 > > LIST_HEAD(ima_measurements); /* list of all measurements */ > +#ifdef CONFIG_IMA_KEXEC > +static unsigned long binary_runtime_size; > +#else > +static unsigned long binary_runtime_size = ULONG_MAX; > +#endif > > /* key: inode (before secure-hashing a file) */ > struct ima_h_table ima_htable = { > @@ -64,6 +69,24 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 > *digest_value, > return ret; > } > > +/* > + * Calculate the memory required for serializing a single > + * binary_runtime_measurement list entry, which contains a > + * couple of variable length fields (e.g template name and data). > + */ > +static int get_binary_runtime_size(struct ima_template_entry *entry) > +{ > + int size = 0; > + > + size += sizeof(u32);/* pcr */ > + size += sizeof(entry->digest); > + size += sizeof(int);/* template name size field */ > + size += strlen(entry->template_desc->name); > + size += sizeof(entry->template_data_len); > + size += entry->template_data_len; > + return size; > +} > + strlen returns len without '\0'. I cannot see how you would know how to read it back? > /* ima_add_template_entry helper function: > * - Add template entry to the measurement list and hash table, for > * all entries except those carried across kexec. > @@ -90,9 +113,30 @@ static int ima_add_digest_entry(struct ima_template_entry > *entry, int flags) > key = ima_hash_key(entry->digest); > hlist_add_head_rcu(>hnext, _htable.queue[key]); > } > + > + if (binary_runtime_size != ULONG_MAX) { > + int size; > + > + size = get_binary_runtime_size(entry); > + binary_runtime_size = (binary_runtime_size < ULONG_MAX - > size) ? > +binary_runtime_size + size : ULONG_MAX; > + } > return 0; > } > > +/* > + * Return the amount of memory required for serializing the > + * entire binary_runtime_measurement list, including the ima_kexec_hdr > + * structure. > + */ > +unsigned long ima_get_binary_runtime_size(void) > +{ > + if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr))) > + return ULONG_MAX; > + else > + return binary_runtime_size + sizeof(struct ima_kexec_hdr); > +}; > + > static int ima_pcr_extend(const u8 *hash, int pcr) > { > int result = 0; > @@ -106,8 +150,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr) > return
Re: [Linux-ima-devel] [PATCH v6 04/10] ima: maintain memory size needed for serializing the measurement list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann wrote: > From: Mimi Zohar > > In preparation for serializing the binary_runtime_measurements, this patch > maintains the amount of memory required. > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > > Changelog v3: > - include the ima_kexec_hdr size in the binary_runtime_measurement size. > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Kconfig | 12 + > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_queue.c | 53 > -- > 3 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 5487827fa86c..370eb2f4dd37 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -27,6 +27,18 @@ config IMA > to learn more about IMA. > If unsure, say N. > > +config IMA_KEXEC > + bool "Enable carrying the IMA measurement list across a soft boot" > + depends on IMA && TCG_TPM && HAVE_IMA_KEXEC > + default n > + help > + TPM PCRs are only reset on a hard reboot. In order to validate > + a TPM's quote after a soft boot, the IMA measurement list of the > + running kernel must be saved and restored on boot. > + > + Depending on the IMA policy, the measurement list can grow to > + be very large. > + > config IMA_MEASURE_PCR_IDX > int > depends on IMA > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 51dc8d57d64d..ea1dcc452911 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -143,6 +143,7 @@ void ima_print_digest(struct seq_file *m, u8 *digest, u32 > size); > struct ima_template_desc *ima_template_desc_current(void); > int ima_restore_measurement_entry(struct ima_template_entry *entry); > int ima_restore_measurement_list(loff_t bufsize, void *buf); > +unsigned long ima_get_binary_runtime_size(void); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_queue.c > b/security/integrity/ima/ima_queue.c > index 12d1b040bca9..3a3cc2a45645 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -29,6 +29,11 @@ > #define AUDIT_CAUSE_LEN_MAX 32 > > LIST_HEAD(ima_measurements); /* list of all measurements */ > +#ifdef CONFIG_IMA_KEXEC > +static unsigned long binary_runtime_size; > +#else > +static unsigned long binary_runtime_size = ULONG_MAX; > +#endif > > /* key: inode (before secure-hashing a file) */ > struct ima_h_table ima_htable = { > @@ -64,6 +69,24 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 > *digest_value, > return ret; > } > > +/* > + * Calculate the memory required for serializing a single > + * binary_runtime_measurement list entry, which contains a > + * couple of variable length fields (e.g template name and data). > + */ > +static int get_binary_runtime_size(struct ima_template_entry *entry) > +{ > + int size = 0; > + > + size += sizeof(u32);/* pcr */ > + size += sizeof(entry->digest); > + size += sizeof(int);/* template name size field */ > + size += strlen(entry->template_desc->name); > + size += sizeof(entry->template_data_len); > + size += entry->template_data_len; > + return size; > +} > + strlen returns len without '\0'. I cannot see how you would know how to read it back? > /* ima_add_template_entry helper function: > * - Add template entry to the measurement list and hash table, for > * all entries except those carried across kexec. > @@ -90,9 +113,30 @@ static int ima_add_digest_entry(struct ima_template_entry > *entry, int flags) > key = ima_hash_key(entry->digest); > hlist_add_head_rcu(>hnext, _htable.queue[key]); > } > + > + if (binary_runtime_size != ULONG_MAX) { > + int size; > + > + size = get_binary_runtime_size(entry); > + binary_runtime_size = (binary_runtime_size < ULONG_MAX - > size) ? > +binary_runtime_size + size : ULONG_MAX; > + } > return 0; > } > > +/* > + * Return the amount of memory required for serializing the > + * entire binary_runtime_measurement list, including the ima_kexec_hdr > + * structure. > + */ > +unsigned long ima_get_binary_runtime_size(void) > +{ > + if (binary_runtime_size >= (ULONG_MAX - sizeof(struct ima_kexec_hdr))) > + return ULONG_MAX; > + else > + return binary_runtime_size + sizeof(struct ima_kexec_hdr); > +}; > + > static int ima_pcr_extend(const u8 *hash, int pcr) > { > int result = 0; > @@ -106,8 +150,13 @@ static int ima_pcr_extend(const u8 *hash, int pcr) > return result; > } > > -/* Add template entry to the measurement list and hash table, > -
Re: [Linux-ima-devel] [PATCH v6 03/10] ima: permit duplicate measurement list entries
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermannwrote: > From: Mimi Zohar > > Measurements carried across kexec need to be added to the IMA > measurement list, but should not prevent measurements of the newly > booted kernel from being added to the measurement list. This patch > adds support for allowing duplicate measurements. > > The "boot_aggregate" measurement entry is the delimiter between soft > boots. > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/ima_queue.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/security/integrity/ima/ima_queue.c > b/security/integrity/ima/ima_queue.c > index 4b1bb7787839..12d1b040bca9 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -65,11 +65,12 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 > *digest_value, > } > > /* ima_add_template_entry helper function: > - * - Add template entry to measurement list and hash table. > + * - Add template entry to the measurement list and hash table, for > + * all entries except those carried across kexec. > * > * (Called with ima_extend_list_mutex held.) > */ > -static int ima_add_digest_entry(struct ima_template_entry *entry) > +static int ima_add_digest_entry(struct ima_template_entry *entry, int flags) > { > struct ima_queue_entry *qe; > unsigned int key; > @@ -85,8 +86,10 @@ static int ima_add_digest_entry(struct ima_template_entry > *entry) > list_add_tail_rcu(>later, _measurements); > > atomic_long_inc(_htable.len); > - key = ima_hash_key(entry->digest); > - hlist_add_head_rcu(>hnext, _htable.queue[key]); > + if (flags) { It looks lile "bool", not flags in fact. > + key = ima_hash_key(entry->digest); > + hlist_add_head_rcu(>hnext, _htable.queue[key]); > + } > return 0; > } > > @@ -126,7 +129,7 @@ int ima_add_template_entry(struct ima_template_entry > *entry, int violation, > } > } > > - result = ima_add_digest_entry(entry); > + result = ima_add_digest_entry(entry, 1); > if (result < 0) { > audit_cause = "ENOMEM"; > audit_info = 0; > @@ -155,7 +158,7 @@ int ima_restore_measurement_entry(struct > ima_template_entry *entry) > int result = 0; > > mutex_lock(_extend_list_mutex); > - result = ima_add_digest_entry(entry); > + result = ima_add_digest_entry(entry, 0); > mutex_unlock(_extend_list_mutex); > return result; > } > -- > 2.7.4 > > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Linux-ima-devel mailing list > linux-ima-de...@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-ima-devel -- Thanks, Dmitry
Re: [Linux-ima-devel] [PATCH v6 03/10] ima: permit duplicate measurement list entries
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann wrote: > From: Mimi Zohar > > Measurements carried across kexec need to be added to the IMA > measurement list, but should not prevent measurements of the newly > booted kernel from being added to the measurement list. This patch > adds support for allowing duplicate measurements. > > The "boot_aggregate" measurement entry is the delimiter between soft > boots. > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/ima_queue.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/security/integrity/ima/ima_queue.c > b/security/integrity/ima/ima_queue.c > index 4b1bb7787839..12d1b040bca9 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -65,11 +65,12 @@ static struct ima_queue_entry *ima_lookup_digest_entry(u8 > *digest_value, > } > > /* ima_add_template_entry helper function: > - * - Add template entry to measurement list and hash table. > + * - Add template entry to the measurement list and hash table, for > + * all entries except those carried across kexec. > * > * (Called with ima_extend_list_mutex held.) > */ > -static int ima_add_digest_entry(struct ima_template_entry *entry) > +static int ima_add_digest_entry(struct ima_template_entry *entry, int flags) > { > struct ima_queue_entry *qe; > unsigned int key; > @@ -85,8 +86,10 @@ static int ima_add_digest_entry(struct ima_template_entry > *entry) > list_add_tail_rcu(>later, _measurements); > > atomic_long_inc(_htable.len); > - key = ima_hash_key(entry->digest); > - hlist_add_head_rcu(>hnext, _htable.queue[key]); > + if (flags) { It looks lile "bool", not flags in fact. > + key = ima_hash_key(entry->digest); > + hlist_add_head_rcu(>hnext, _htable.queue[key]); > + } > return 0; > } > > @@ -126,7 +129,7 @@ int ima_add_template_entry(struct ima_template_entry > *entry, int violation, > } > } > > - result = ima_add_digest_entry(entry); > + result = ima_add_digest_entry(entry, 1); > if (result < 0) { > audit_cause = "ENOMEM"; > audit_info = 0; > @@ -155,7 +158,7 @@ int ima_restore_measurement_entry(struct > ima_template_entry *entry) > int result = 0; > > mutex_lock(_extend_list_mutex); > - result = ima_add_digest_entry(entry); > + result = ima_add_digest_entry(entry, 0); > mutex_unlock(_extend_list_mutex); > return result; > } > -- > 2.7.4 > > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > Linux-ima-devel mailing list > linux-ima-de...@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-ima-devel -- Thanks, Dmitry
Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermannwrote: > From: Mimi Zohar > > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and restored on boot. This patch > restores the measurement list. > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago) > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago) > - remove unnecessary includes from ima_kexec.c (Thiago) > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King) > > Changelog v2: > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman) > - defined missing ima_load_kexec_buffer() stub function > > Changelog v1: > - call ima_load_kexec_buffer() (Thiago) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 21 + > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c| 44 + > security/integrity/ima/ima_queue.c| 10 ++ > security/integrity/ima/ima_template.c | 170 > ++ > 6 files changed, 248 insertions(+) > > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 9aeaedad1e2b..29f198bde02b 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ > ima_policy.o ima_template.o ima_template_lib.o > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index db25f54a04fe..51dc8d57d64d 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -28,6 +28,10 @@ > > #include "../integrity.h" > > +#ifdef CONFIG_HAVE_IMA_KEXEC > +#include > +#endif > + > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; > @@ -102,6 +106,21 @@ struct ima_queue_entry { > }; > extern struct list_head ima_measurements; /* list of all measurements */ > > +/* Some details preceding the binary serialized measurement list */ > +struct ima_kexec_hdr { > + u16 version; > + u16 _reserved0; > + u32 _reserved1; > + u64 buffer_size; > + u64 count; > +}; > + > +#ifdef CONFIG_HAVE_IMA_KEXEC > +void ima_load_kexec_buffer(void); > +#else > +static inline void ima_load_kexec_buffer(void) {} > +#endif /* CONFIG_HAVE_IMA_KEXEC */ > + > /* Internal IMA function definitions */ > int ima_init(void); > int ima_fs_init(void); > @@ -122,6 +141,8 @@ int ima_init_crypto(void); > void ima_putc(struct seq_file *m, void *data, int datalen); > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); > struct ima_template_desc *ima_template_desc_current(void); > +int ima_restore_measurement_entry(struct ima_template_entry *entry); > +int ima_restore_measurement_list(loff_t bufsize, void *buf); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > index 32912bd54ead..3ba0ca49cba6 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -128,6 +128,8 @@ int __init ima_init(void) > if (rc != 0) > return rc; > > + ima_load_kexec_buffer(); > + > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry > */ > if (rc != 0) > return rc; > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > new file mode 100644 > index ..36afd0fe9747 > --- /dev/null > +++ b/security/integrity/ima/ima_kexec.c > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2016 IBM Corporation > + * > + * Authors: > + * Thiago Jung Bauermann > + * Mimi Zohar > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include "ima.h" > + > +/* > + * Restore the measurement list from the previous kernel. > + */ > +void ima_load_kexec_buffer(void) > +{ > + void *kexec_buffer = NULL; > + size_t kexec_buffer_size = 0; > + int rc; > + > + rc = ima_get_kexec_buffer(_buffer, _buffer_size); > +
Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list
On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann wrote: > From: Mimi Zohar > > The TPM PCRs are only reset on a hard reboot. In order to validate a > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list > of the running kernel must be saved and restored on boot. This patch > restores the measurement list. > > Changelog v5: > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago) > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago) > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago) > - remove unnecessary includes from ima_kexec.c (Thiago) > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King) > > Changelog v2: > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman) > - defined missing ima_load_kexec_buffer() stub function > > Changelog v1: > - call ima_load_kexec_buffer() (Thiago) > > Signed-off-by: Mimi Zohar > --- > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima.h | 21 + > security/integrity/ima/ima_init.c | 2 + > security/integrity/ima/ima_kexec.c| 44 + > security/integrity/ima/ima_queue.c| 10 ++ > security/integrity/ima/ima_template.c | 170 > ++ > 6 files changed, 248 insertions(+) > > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 9aeaedad1e2b..29f198bde02b 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \ > ima_policy.o ima_template.o ima_template_lib.o > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index db25f54a04fe..51dc8d57d64d 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -28,6 +28,10 @@ > > #include "../integrity.h" > > +#ifdef CONFIG_HAVE_IMA_KEXEC > +#include > +#endif > + > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN, > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII }; > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 }; > @@ -102,6 +106,21 @@ struct ima_queue_entry { > }; > extern struct list_head ima_measurements; /* list of all measurements */ > > +/* Some details preceding the binary serialized measurement list */ > +struct ima_kexec_hdr { > + u16 version; > + u16 _reserved0; > + u32 _reserved1; > + u64 buffer_size; > + u64 count; > +}; > + > +#ifdef CONFIG_HAVE_IMA_KEXEC > +void ima_load_kexec_buffer(void); > +#else > +static inline void ima_load_kexec_buffer(void) {} > +#endif /* CONFIG_HAVE_IMA_KEXEC */ > + > /* Internal IMA function definitions */ > int ima_init(void); > int ima_fs_init(void); > @@ -122,6 +141,8 @@ int ima_init_crypto(void); > void ima_putc(struct seq_file *m, void *data, int datalen); > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size); > struct ima_template_desc *ima_template_desc_current(void); > +int ima_restore_measurement_entry(struct ima_template_entry *entry); > +int ima_restore_measurement_list(loff_t bufsize, void *buf); > int ima_init_template(void); > > /* > diff --git a/security/integrity/ima/ima_init.c > b/security/integrity/ima/ima_init.c > index 32912bd54ead..3ba0ca49cba6 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -128,6 +128,8 @@ int __init ima_init(void) > if (rc != 0) > return rc; > > + ima_load_kexec_buffer(); > + > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry > */ > if (rc != 0) > return rc; > diff --git a/security/integrity/ima/ima_kexec.c > b/security/integrity/ima/ima_kexec.c > new file mode 100644 > index ..36afd0fe9747 > --- /dev/null > +++ b/security/integrity/ima/ima_kexec.c > @@ -0,0 +1,44 @@ > +/* > + * Copyright (C) 2016 IBM Corporation > + * > + * Authors: > + * Thiago Jung Bauermann > + * Mimi Zohar > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include "ima.h" > + > +/* > + * Restore the measurement list from the previous kernel. > + */ > +void ima_load_kexec_buffer(void) > +{ > + void *kexec_buffer = NULL; > + size_t kexec_buffer_size = 0; > + int rc; > + > + rc = ima_get_kexec_buffer(_buffer, _buffer_size); > + switch (rc) { > + case 0: > + rc = ima_restore_measurement_list(kexec_buffer_size, > +
RE: 'select' on deleted KEYS_DEBUG_PROC_KEYS option
Hi, Yes, please make a patch. Thanks for noticing, Dmitry From: Andreas Ziegler [andreas.zieg...@fau.de] Sent: Tuesday, January 26, 2016 5:39 PM To: Dmitry Kasatkin Cc: David Howells; James Morris; Serge E. Hallyn; linux-kernel@vger.kernel.org Subject: 'select' on deleted KEYS_DEBUG_PROC_KEYS option Hi Dmitry, your commit f4dc37785e9b ("integrity: define '.evm' as a builtin 'trusted' keyring") introduces the INTEGRITY_TRUSTED_KEYRING Kconfig option, which has a 'select' to KEYS_DEBUG_PROC_KEYS. This option, however, has been removed over a year ago by commit dabd39cc2fb1 ("KEYS: Make /proc/keys unconditional if CONFIG_KEYS=y") and -- besides the 'select' mentioned above -- only occurs inside (outdated) defconfigs. Should I prepare a simple patch removing the select? Best regards, Andreas
RE: 'select' on deleted KEYS_DEBUG_PROC_KEYS option
Hi, Yes, please make a patch. Thanks for noticing, Dmitry From: Andreas Ziegler [andreas.zieg...@fau.de] Sent: Tuesday, January 26, 2016 5:39 PM To: Dmitry Kasatkin Cc: David Howells; James Morris; Serge E. Hallyn; linux-kernel@vger.kernel.org Subject: 'select' on deleted KEYS_DEBUG_PROC_KEYS option Hi Dmitry, your commit f4dc37785e9b ("integrity: define '.evm' as a builtin 'trusted' keyring") introduces the INTEGRITY_TRUSTED_KEYRING Kconfig option, which has a 'select' to KEYS_DEBUG_PROC_KEYS. This option, however, has been removed over a year ago by commit dabd39cc2fb1 ("KEYS: Make /proc/keys unconditional if CONFIG_KEYS=y") and -- besides the 'select' mentioned above -- only occurs inside (outdated) defconfigs. Should I prepare a simple patch removing the select? Best regards, Andreas
Re: [PATCHv3 4/6] evm: provide a function to set EVM key from the kernel
Hi, Updated in the patch. http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next Dmitry On Fri, Oct 23, 2015 at 9:30 PM, Mimi Zohar wrote: > On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote: >> Crypto HW kernel module can possibly initialize EVM key from the >> kernel __init code to enable EVM before calling 'init' process. >> This patch provide a function evm_set_key() which can be used to >> set custom key directly to EVM without using KEY subsystem. > > Thanks, Dmitry. There's a minor comment inline. >> >> Changes in v3: >> * error reporting moved to evm_set_key >> * EVM_INIT_HMAC moved to evm_set_key >> * added bitop to prevent key setting race >> >> Changes in v2: >> * use size_t for key size instead of signed int >> * provide EVM_MAX_KEY_SIZE macro in >> * provide EVM_MIN_KEY_SIZE macro in >> >> Signed-off-by: Dmitry Kasatkin >> --- >> include/linux/evm.h | 7 ++ >> security/integrity/evm/evm_crypto.c | 47 >> +++-- >> security/integrity/evm/evm_secfs.c | 10 +++- >> 3 files changed, 50 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/evm.h b/include/linux/evm.h >> index 1fcb88c..35ed9a8 100644 >> --- a/include/linux/evm.h >> +++ b/include/linux/evm.h >> @@ -14,6 +14,7 @@ >> struct integrity_iint_cache; >> >> #ifdef CONFIG_EVM >> +extern int evm_set_key(void *key, size_t keylen); >> extern enum integrity_status evm_verifyxattr(struct dentry *dentry, >>const char *xattr_name, >>void *xattr_value, >> @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname) >> } >> #endif >> #else >> + >> +static inline int evm_set_key(void *key, size_t keylen) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> #ifdef CONFIG_INTEGRITY >> static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, >> const char *xattr_name, >> diff --git a/security/integrity/evm/evm_crypto.c >> b/security/integrity/evm/evm_crypto.c >> index 34e1a6f..7aec93e 100644 >> --- a/security/integrity/evm/evm_crypto.c >> +++ b/security/integrity/evm/evm_crypto.c >> @@ -18,6 +18,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include "evm.h" >> @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm; >> >> static DEFINE_MUTEX(mutex); >> >> +#define EVM_SET_KEY_BUSY 0 >> + >> +static unsigned long evm_set_key_flags; >> + >> +/* evm_set_key - set EVM HMAC key from the kernel >> + * > > For exported functions this should be "kernel-doc" format. > > Mimi > >> + * This function allows to set EVM HMAC key from the kernel >> + * without using key subsystem 'encrypted' keys. It can be used >> + * by the crypto HW kernel module which has own way of managing >> + * keys. >> + * >> + * key length should be between 32 and 128 bytes long >> + */ >> +int evm_set_key(void *key, size_t keylen) >> +{ >> + int rc; >> + >> + rc = -EBUSY; >> + if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) >> + goto busy; >> + rc = -EINVAL; >> + if (keylen > MAX_KEY_SIZE) >> + goto inval; >> + memcpy(evmkey, key, keylen); >> + evm_initialized |= EVM_INIT_HMAC; >> + pr_info("key initialized\n"); >> + return 0; >> +inval: >> + clear_bit(EVM_SET_KEY_BUSY, _set_key_flags); >> +busy: >> + pr_err("key initialization failed\n"); >> + return rc; >> +} >> +EXPORT_SYMBOL_GPL(evm_set_key); >> + >> static struct shash_desc *init_desc(char type) >> { >> long rc; >> @@ -242,7 +278,7 @@ int evm_init_key(void) >> { >> struct key *evm_key; >> struct encrypted_key_payload *ekp; >> - int rc = 0; >> + int rc; >> >> evm_key = request_key(_type_encrypted, EVMKEY, NULL); >> if (IS_ERR(evm_key)) >> @@ -250,12 +286,9 @@ int evm_init_key(void) >> >> down_read(_key->sem); >> ekp = evm_key->payload.data; >> - if (ekp->decrypted_datalen > MAX_KEY_SIZE) { >> - rc = -EINVAL; >> - goto out; >> - } >> -
Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded
Hi, I added error printing to the patch http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next Dmitry On Fri, Oct 23, 2015 at 9:31 PM, Mimi Zohar wrote: > On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote: >> In order to enable EVM before starting 'init' process, >> evm_initialized needs to be non-zero. Before it was >> indicating that HMAC key is loaded. When EVM loads >> X509 before calling 'init', it is possible to enable >> EVM to start signature based verification. >> >> This patch defines bits to enable EVM if key of any type >> is loaded. > > Thanks, Dmitry. There's one comment inline. > >> Changes in v2: >> * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC >> * EVM_STATE_X509_SET replaced by EVM_INIT_X509 >> >> Signed-off-by: Dmitry Kasatkin >> --- >> security/integrity/evm/evm.h| 3 +++ >> security/integrity/evm/evm_crypto.c | 2 ++ >> security/integrity/evm/evm_main.c | 6 +- >> security/integrity/evm/evm_secfs.c | 4 ++-- >> 4 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h >> index 88bfe77..f5f1272 100644 >> --- a/security/integrity/evm/evm.h >> +++ b/security/integrity/evm/evm.h >> @@ -21,6 +21,9 @@ >> >> #include "../integrity.h" >> >> +#define EVM_INIT_HMAC0x0001 >> +#define EVM_INIT_X5090x0002 >> + >> extern int evm_initialized; >> extern char *evm_hmac; >> extern char *evm_hash; >> diff --git a/security/integrity/evm/evm_crypto.c >> b/security/integrity/evm/evm_crypto.c >> index 159ef3e..34e1a6f 100644 >> --- a/security/integrity/evm/evm_crypto.c >> +++ b/security/integrity/evm/evm_crypto.c >> @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type) >> struct shash_desc *desc; >> >> if (type == EVM_XATTR_HMAC) { >> + if (!(evm_initialized & EVM_INIT_HMAC)) >> + return ERR_PTR(-ENOKEY); > > init_desc() is called from a couple of different places. In some > instances, like when converting from a signature to an hmac, if > init_desc() fails, the xattr isn't converted to an HMAC. No big deal. > But there are other cases, like when a protected xattr is modified, > failing the write will make the file inaccessible. Does there need to > be an error msg of some sort or an audit msg? > > Mimi > >> tfm = _tfm; >> algo = evm_hmac; >> } else { >> diff --git a/security/integrity/evm/evm_main.c >> b/security/integrity/evm/evm_main.c >> index 519de0a..420d94d 100644 >> --- a/security/integrity/evm/evm_main.c >> +++ b/security/integrity/evm/evm_main.c >> @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security); >> #ifdef CONFIG_EVM_LOAD_X509 >> void __init evm_load_x509(void) >> { >> - integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); >> + int rc; >> + >> + rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); >> + if (!rc) >> + evm_initialized |= EVM_INIT_X509; >> } >> #endif >> >> diff --git a/security/integrity/evm/evm_secfs.c >> b/security/integrity/evm/evm_secfs.c >> index cf12a04..3f775df 100644 >> --- a/security/integrity/evm/evm_secfs.c >> +++ b/security/integrity/evm/evm_secfs.c >> @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char >> __user *buf, >> char temp[80]; >> int i, error; >> >> - if (!capable(CAP_SYS_ADMIN) || evm_initialized) >> + if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) >> return -EPERM; >> >> if (count >= sizeof(temp) || count == 0) >> @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char >> __user *buf, >> >> error = evm_init_key(); >> if (!error) { >> - evm_initialized = 1; >> + evm_initialized |= EVM_INIT_HMAC; >> pr_info("initialized\n"); >> } else >> pr_err("initialization failed\n"); > > -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded
Hi, I added error printing to the patch http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next Dmitry On Fri, Oct 23, 2015 at 9:31 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote: > On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote: >> In order to enable EVM before starting 'init' process, >> evm_initialized needs to be non-zero. Before it was >> indicating that HMAC key is loaded. When EVM loads >> X509 before calling 'init', it is possible to enable >> EVM to start signature based verification. >> >> This patch defines bits to enable EVM if key of any type >> is loaded. > > Thanks, Dmitry. There's one comment inline. > >> Changes in v2: >> * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC >> * EVM_STATE_X509_SET replaced by EVM_INIT_X509 >> >> Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> >> --- >> security/integrity/evm/evm.h| 3 +++ >> security/integrity/evm/evm_crypto.c | 2 ++ >> security/integrity/evm/evm_main.c | 6 +- >> security/integrity/evm/evm_secfs.c | 4 ++-- >> 4 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h >> index 88bfe77..f5f1272 100644 >> --- a/security/integrity/evm/evm.h >> +++ b/security/integrity/evm/evm.h >> @@ -21,6 +21,9 @@ >> >> #include "../integrity.h" >> >> +#define EVM_INIT_HMAC0x0001 >> +#define EVM_INIT_X5090x0002 >> + >> extern int evm_initialized; >> extern char *evm_hmac; >> extern char *evm_hash; >> diff --git a/security/integrity/evm/evm_crypto.c >> b/security/integrity/evm/evm_crypto.c >> index 159ef3e..34e1a6f 100644 >> --- a/security/integrity/evm/evm_crypto.c >> +++ b/security/integrity/evm/evm_crypto.c >> @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type) >> struct shash_desc *desc; >> >> if (type == EVM_XATTR_HMAC) { >> + if (!(evm_initialized & EVM_INIT_HMAC)) >> + return ERR_PTR(-ENOKEY); > > init_desc() is called from a couple of different places. In some > instances, like when converting from a signature to an hmac, if > init_desc() fails, the xattr isn't converted to an HMAC. No big deal. > But there are other cases, like when a protected xattr is modified, > failing the write will make the file inaccessible. Does there need to > be an error msg of some sort or an audit msg? > > Mimi > >> tfm = _tfm; >> algo = evm_hmac; >> } else { >> diff --git a/security/integrity/evm/evm_main.c >> b/security/integrity/evm/evm_main.c >> index 519de0a..420d94d 100644 >> --- a/security/integrity/evm/evm_main.c >> +++ b/security/integrity/evm/evm_main.c >> @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security); >> #ifdef CONFIG_EVM_LOAD_X509 >> void __init evm_load_x509(void) >> { >> - integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); >> + int rc; >> + >> + rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); >> + if (!rc) >> + evm_initialized |= EVM_INIT_X509; >> } >> #endif >> >> diff --git a/security/integrity/evm/evm_secfs.c >> b/security/integrity/evm/evm_secfs.c >> index cf12a04..3f775df 100644 >> --- a/security/integrity/evm/evm_secfs.c >> +++ b/security/integrity/evm/evm_secfs.c >> @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char >> __user *buf, >> char temp[80]; >> int i, error; >> >> - if (!capable(CAP_SYS_ADMIN) || evm_initialized) >> + if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) >> return -EPERM; >> >> if (count >= sizeof(temp) || count == 0) >> @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char >> __user *buf, >> >> error = evm_init_key(); >> if (!error) { >> - evm_initialized = 1; >> + evm_initialized |= EVM_INIT_HMAC; >> pr_info("initialized\n"); >> } else >> pr_err("initialization failed\n"); > > -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv3 4/6] evm: provide a function to set EVM key from the kernel
Hi, Updated in the patch. http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=ima-next Dmitry On Fri, Oct 23, 2015 at 9:30 PM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote: > On Thu, 2015-10-22 at 21:49 +0300, Dmitry Kasatkin wrote: >> Crypto HW kernel module can possibly initialize EVM key from the >> kernel __init code to enable EVM before calling 'init' process. >> This patch provide a function evm_set_key() which can be used to >> set custom key directly to EVM without using KEY subsystem. > > Thanks, Dmitry. There's a minor comment inline. >> >> Changes in v3: >> * error reporting moved to evm_set_key >> * EVM_INIT_HMAC moved to evm_set_key >> * added bitop to prevent key setting race >> >> Changes in v2: >> * use size_t for key size instead of signed int >> * provide EVM_MAX_KEY_SIZE macro in >> * provide EVM_MIN_KEY_SIZE macro in >> >> Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> >> --- >> include/linux/evm.h | 7 ++ >> security/integrity/evm/evm_crypto.c | 47 >> +++-- >> security/integrity/evm/evm_secfs.c | 10 +++- >> 3 files changed, 50 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/evm.h b/include/linux/evm.h >> index 1fcb88c..35ed9a8 100644 >> --- a/include/linux/evm.h >> +++ b/include/linux/evm.h >> @@ -14,6 +14,7 @@ >> struct integrity_iint_cache; >> >> #ifdef CONFIG_EVM >> +extern int evm_set_key(void *key, size_t keylen); >> extern enum integrity_status evm_verifyxattr(struct dentry *dentry, >>const char *xattr_name, >>void *xattr_value, >> @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname) >> } >> #endif >> #else >> + >> +static inline int evm_set_key(void *key, size_t keylen) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> #ifdef CONFIG_INTEGRITY >> static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, >> const char *xattr_name, >> diff --git a/security/integrity/evm/evm_crypto.c >> b/security/integrity/evm/evm_crypto.c >> index 34e1a6f..7aec93e 100644 >> --- a/security/integrity/evm/evm_crypto.c >> +++ b/security/integrity/evm/evm_crypto.c >> @@ -18,6 +18,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include "evm.h" >> @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm; >> >> static DEFINE_MUTEX(mutex); >> >> +#define EVM_SET_KEY_BUSY 0 >> + >> +static unsigned long evm_set_key_flags; >> + >> +/* evm_set_key - set EVM HMAC key from the kernel >> + * > > For exported functions this should be "kernel-doc" format. > > Mimi > >> + * This function allows to set EVM HMAC key from the kernel >> + * without using key subsystem 'encrypted' keys. It can be used >> + * by the crypto HW kernel module which has own way of managing >> + * keys. >> + * >> + * key length should be between 32 and 128 bytes long >> + */ >> +int evm_set_key(void *key, size_t keylen) >> +{ >> + int rc; >> + >> + rc = -EBUSY; >> + if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) >> + goto busy; >> + rc = -EINVAL; >> + if (keylen > MAX_KEY_SIZE) >> + goto inval; >> + memcpy(evmkey, key, keylen); >> + evm_initialized |= EVM_INIT_HMAC; >> + pr_info("key initialized\n"); >> + return 0; >> +inval: >> + clear_bit(EVM_SET_KEY_BUSY, _set_key_flags); >> +busy: >> + pr_err("key initialization failed\n"); >> + return rc; >> +} >> +EXPORT_SYMBOL_GPL(evm_set_key); >> + >> static struct shash_desc *init_desc(char type) >> { >> long rc; >> @@ -242,7 +278,7 @@ int evm_init_key(void) >> { >> struct key *evm_key; >> struct encrypted_key_payload *ekp; >> - int rc = 0; >> + int rc; >> >> evm_key = request_key(_type_encrypted, EVMKEY, NULL); >> if (IS_ERR(evm_key)) >> @@ -250,12 +286,9 @@ int evm_init_key(void) >> >> down_read(_key->sem); >> ekp = evm_key->payload.data; >> - if (ekp->decrypted_datalen > MAX_KEY_SIZE) { >> - rc = -EINVAL; >> -
RE: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
From: Petko Manolov [pet...@mip-labs.com] Sent: Friday, October 23, 2015 4:05 PM To: Dmitry Kasatkin Cc: zo...@linux.vnet.ibm.com; linux-ima-de...@lists.sourceforge.net; linux-security-mod...@vger.kernel.org; linux-kernel@vger.kernel.org; Dmitry Kasatkin Subject: Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring On 15-10-22 21:49:25, Dmitry Kasatkin wrote: > Require all keys added to the EVM keyring be signed by an > existing trusted key on the system trusted keyring. > > This patch also switches IMA to use integrity_init_keyring(). > > Changes in v3: > * Added 'init_keyring' config based variable to skip initializing > keyring instead of using __integrity_init_keyring() wrapper. > * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING > > Changes in v2: > * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common > CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option > * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config > file compatibility. (Mimi Zohar) > > Signed-off-by: Dmitry Kasatkin > --- > security/integrity/Kconfig| 11 +++ > security/integrity/digsig.c | 14 -- > security/integrity/evm/evm_main.c | 8 +--- > security/integrity/ima/Kconfig| 5 - > security/integrity/ima/ima.h | 12 > security/integrity/ima/ima_init.c | 2 +- > security/integrity/integrity.h| 5 ++--- > 7 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index 73c457b..21d7568 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS > This option enables digital signature verification using > asymmetric keys. > > +config INTEGRITY_TRUSTED_KEYRING > + bool "Require all keys on the integrity keyrings be signed" > + depends on SYSTEM_TRUSTED_KEYRING > + depends on INTEGRITY_ASYMMETRIC_KEYS > + select KEYS_DEBUG_PROC_KEYS > + default y > + help > +This option requires that all keys added to the .ima and > +.evm keyrings be signed by a key on the system trusted > +keyring. > + > config INTEGRITY_AUDIT > bool "Enables integrity auditing support " > depends on AUDIT > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index 5be9ffb..8ef1511 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -24,15 +24,22 @@ > static struct key *keyring[INTEGRITY_KEYRING_MAX]; > > static const char *keyring_name[INTEGRITY_KEYRING_MAX] = { > +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING > "_evm", > - "_module", > -#ifndef CONFIG_IMA_TRUSTED_KEYRING > "_ima", > #else > + ".evm", > ".ima", > #endif > + "_module", > }; > > +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING > +static bool init_keyring __initdata = true; > +#else > +static bool init_keyring __initdata; > +#endif > + > int integrity_digsig_verify(const unsigned int id, const char *sig, int > siglen, > const char *digest, int digestlen) > { > @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id) > const struct cred *cred = current_cred(); > int err = 0; > > + if (!init_keyring) > + return 0; > + > keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0), > KGIDT_INIT(0), cred, > ((KEY_POS_ALL & ~KEY_POS_SETATTR) | > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 1334e02..75b7e30 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -478,15 +478,17 @@ static int __init init_evm(void) > > evm_init_config(); > > + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM); > + if (error) > + return error; > + > error = evm_init_secfs(); > if (error < 0) { > pr_info("Error registering secfs\n"); > - goto err; > + return error; > } > > return 0; > -err: > - return error; > } > > /* > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index df30334..a292b88 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -123,14 +123,17 @@ config IMA_APPRAISE > If unsure, say N. > > conf
RE: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
From: Petko Manolov [pet...@mip-labs.com] Sent: Friday, October 23, 2015 4:05 PM To: Dmitry Kasatkin Cc: zo...@linux.vnet.ibm.com; linux-ima-de...@lists.sourceforge.net; linux-security-mod...@vger.kernel.org; linux-kernel@vger.kernel.org; Dmitry Kasatkin Subject: Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring On 15-10-22 21:49:25, Dmitry Kasatkin wrote: > Require all keys added to the EVM keyring be signed by an > existing trusted key on the system trusted keyring. > > This patch also switches IMA to use integrity_init_keyring(). > > Changes in v3: > * Added 'init_keyring' config based variable to skip initializing > keyring instead of using __integrity_init_keyring() wrapper. > * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING > > Changes in v2: > * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common > CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option > * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config > file compatibility. (Mimi Zohar) > > Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> > --- > security/integrity/Kconfig| 11 +++ > security/integrity/digsig.c | 14 -- > security/integrity/evm/evm_main.c | 8 +--- > security/integrity/ima/Kconfig| 5 - > security/integrity/ima/ima.h | 12 > security/integrity/ima/ima_init.c | 2 +- > security/integrity/integrity.h| 5 ++--- > 7 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index 73c457b..21d7568 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS > This option enables digital signature verification using > asymmetric keys. > > +config INTEGRITY_TRUSTED_KEYRING > + bool "Require all keys on the integrity keyrings be signed" > + depends on SYSTEM_TRUSTED_KEYRING > + depends on INTEGRITY_ASYMMETRIC_KEYS > + select KEYS_DEBUG_PROC_KEYS > + default y > + help > +This option requires that all keys added to the .ima and > +.evm keyrings be signed by a key on the system trusted > +keyring. > + > config INTEGRITY_AUDIT > bool "Enables integrity auditing support " > depends on AUDIT > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index 5be9ffb..8ef1511 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -24,15 +24,22 @@ > static struct key *keyring[INTEGRITY_KEYRING_MAX]; > > static const char *keyring_name[INTEGRITY_KEYRING_MAX] = { > +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING > "_evm", > - "_module", > -#ifndef CONFIG_IMA_TRUSTED_KEYRING > "_ima", > #else > + ".evm", > ".ima", > #endif > + "_module", > }; > > +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING > +static bool init_keyring __initdata = true; > +#else > +static bool init_keyring __initdata; > +#endif > + > int integrity_digsig_verify(const unsigned int id, const char *sig, int > siglen, > const char *digest, int digestlen) > { > @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id) > const struct cred *cred = current_cred(); > int err = 0; > > + if (!init_keyring) > + return 0; > + > keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0), > KGIDT_INIT(0), cred, > ((KEY_POS_ALL & ~KEY_POS_SETATTR) | > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 1334e02..75b7e30 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -478,15 +478,17 @@ static int __init init_evm(void) > > evm_init_config(); > > + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM); > + if (error) > + return error; > + > error = evm_init_secfs(); > if (error < 0) { > pr_info("Error registering secfs\n"); > - goto err; > + return error; > } > > return 0; > -err: > - return error; > } > > /* > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index df30334..a292b88 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -123,14 +123,17 @@ config IMA_APPRAISE > If unsure,
[PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded
In order to enable EVM before starting 'init' process, evm_initialized needs to be non-zero. Before it was indicating that HMAC key is loaded. When EVM loads X509 before calling 'init', it is possible to enable EVM to start signature based verification. This patch defines bits to enable EVM if key of any type is loaded. Changes in v2: * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC * EVM_STATE_X509_SET replaced by EVM_INIT_X509 Signed-off-by: Dmitry Kasatkin --- security/integrity/evm/evm.h| 3 +++ security/integrity/evm/evm_crypto.c | 2 ++ security/integrity/evm/evm_main.c | 6 +- security/integrity/evm/evm_secfs.c | 4 ++-- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index 88bfe77..f5f1272 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -21,6 +21,9 @@ #include "../integrity.h" +#define EVM_INIT_HMAC 0x0001 +#define EVM_INIT_X509 0x0002 + extern int evm_initialized; extern char *evm_hmac; extern char *evm_hash; diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 159ef3e..34e1a6f 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type) struct shash_desc *desc; if (type == EVM_XATTR_HMAC) { + if (!(evm_initialized & EVM_INIT_HMAC)) + return ERR_PTR(-ENOKEY); tfm = _tfm; algo = evm_hmac; } else { diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 519de0a..420d94d 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security); #ifdef CONFIG_EVM_LOAD_X509 void __init evm_load_x509(void) { - integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); + int rc; + + rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); + if (!rc) + evm_initialized |= EVM_INIT_X509; } #endif diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index cf12a04..3f775df 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, char temp[80]; int i, error; - if (!capable(CAP_SYS_ADMIN) || evm_initialized) + if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) return -EPERM; if (count >= sizeof(temp) || count == 0) @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, error = evm_init_key(); if (!error) { - evm_initialized = 1; + evm_initialized |= EVM_INIT_HMAC; pr_info("initialized\n"); } else pr_err("initialization failed\n"); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 6/6] evm: reset EVM status when file attributes changes
EVM verification status is cached in iint->evm_status and if it was successful, never re-verified again when IMA passes 'iint' to evm_verifyxattr(). When file attribute or extended attributes changes we may wish to re-verify EVM integrity as well. For example, after setting digital signature we may need to re-verify the signature and update iint->flags that there is EVM signature. This patch enables that by resetting evm_status to INTEGRITY_UKNOWN state. Changes in v2: * Flag setting moved to EVM layer Signed-off-by: Dmitry Kasatkin --- security/integrity/evm/evm_main.c | 13 + 1 file changed, 13 insertions(+) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 420d94d..f716025 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -358,6 +358,15 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name) return evm_protect_xattr(dentry, xattr_name, NULL, 0); } +static void evm_reset_status(struct inode *inode) +{ + struct integrity_iint_cache *iint; + + iint = integrity_iint_find(inode); + if (iint) + iint->evm_status = INTEGRITY_UNKNOWN; +} + /** * evm_inode_post_setxattr - update 'security.evm' to reflect the changes * @dentry: pointer to the affected dentry @@ -378,6 +387,8 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, && !posix_xattr_acl(xattr_name))) return; + evm_reset_status(dentry->d_inode); + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); } @@ -396,6 +407,8 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) if (!evm_initialized || !evm_protected_xattr(xattr_name)) return; + evm_reset_status(dentry->d_inode); + evm_update_evmxattr(dentry, xattr_name, NULL, 0); } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 5/6] evm: define EVM key max and min sizes
This patch imposes minimum key size limit. It declares EVM_MIN_KEY_SIZE and EVM_MAX_KEY_SIZE in public header file. Signed-off-by: Dmitry Kasatkin --- include/linux/evm.h | 3 +++ security/integrity/evm/evm_crypto.c | 7 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/linux/evm.h b/include/linux/evm.h index 35ed9a8..0aeedec 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -11,6 +11,9 @@ #include #include +#define EVM_MAX_KEY_SIZE 128 +#define EVM_MIN_KEY_SIZE 64 + struct integrity_iint_cache; #ifdef CONFIG_EVM diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 7aec93e..cfb788c 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -24,9 +24,8 @@ #include "evm.h" #define EVMKEY "evm-key" -#define MAX_KEY_SIZE 128 -static unsigned char evmkey[MAX_KEY_SIZE]; -static int evmkey_len = MAX_KEY_SIZE; +static unsigned char evmkey[EVM_MAX_KEY_SIZE]; +static int evmkey_len = EVM_MAX_KEY_SIZE; struct crypto_shash *hmac_tfm; struct crypto_shash *hash_tfm; @@ -54,7 +53,7 @@ int evm_set_key(void *key, size_t keylen) if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) goto busy; rc = -EINVAL; - if (keylen > MAX_KEY_SIZE) + if (keylen < EVM_MIN_KEY_SIZE || keylen > EVM_MAX_KEY_SIZE) goto inval; memcpy(evmkey, key, keylen); evm_initialized |= EVM_INIT_HMAC; -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
Require all keys added to the EVM keyring be signed by an existing trusted key on the system trusted keyring. This patch also switches IMA to use integrity_init_keyring(). Changes in v3: * Added 'init_keyring' config based variable to skip initializing keyring instead of using __integrity_init_keyring() wrapper. * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING Changes in v2: * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config file compatibility. (Mimi Zohar) Signed-off-by: Dmitry Kasatkin --- security/integrity/Kconfig| 11 +++ security/integrity/digsig.c | 14 -- security/integrity/evm/evm_main.c | 8 +--- security/integrity/ima/Kconfig| 5 - security/integrity/ima/ima.h | 12 security/integrity/ima/ima_init.c | 2 +- security/integrity/integrity.h| 5 ++--- 7 files changed, 35 insertions(+), 22 deletions(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index 73c457b..21d7568 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS This option enables digital signature verification using asymmetric keys. +config INTEGRITY_TRUSTED_KEYRING + bool "Require all keys on the integrity keyrings be signed" + depends on SYSTEM_TRUSTED_KEYRING + depends on INTEGRITY_ASYMMETRIC_KEYS + select KEYS_DEBUG_PROC_KEYS + default y + help + This option requires that all keys added to the .ima and + .evm keyrings be signed by a key on the system trusted + keyring. + config INTEGRITY_AUDIT bool "Enables integrity auditing support " depends on AUDIT diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 5be9ffb..8ef1511 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -24,15 +24,22 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX]; static const char *keyring_name[INTEGRITY_KEYRING_MAX] = { +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING "_evm", - "_module", -#ifndef CONFIG_IMA_TRUSTED_KEYRING "_ima", #else + ".evm", ".ima", #endif + "_module", }; +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING +static bool init_keyring __initdata = true; +#else +static bool init_keyring __initdata; +#endif + int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, const char *digest, int digestlen) { @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id) const struct cred *cred = current_cred(); int err = 0; + if (!init_keyring) + return 0; + keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0), KGIDT_INIT(0), cred, ((KEY_POS_ALL & ~KEY_POS_SETATTR) | diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 1334e02..75b7e30 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -478,15 +478,17 @@ static int __init init_evm(void) evm_init_config(); + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM); + if (error) + return error; + error = evm_init_secfs(); if (error < 0) { pr_info("Error registering secfs\n"); - goto err; + return error; } return 0; -err: - return error; } /* diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index df30334..a292b88 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -123,14 +123,17 @@ config IMA_APPRAISE If unsure, say N. config IMA_TRUSTED_KEYRING - bool "Require all keys on the .ima keyring be signed" + bool "Require all keys on the .ima keyring be signed (deprecated)" depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING depends on INTEGRITY_ASYMMETRIC_KEYS + select INTEGRITY_TRUSTED_KEYRING default y help This option requires that all keys added to the .ima keyring be signed by a key on the system trusted keyring. + This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING + config IMA_LOAD_X509 bool "Load X509 certificate onto the '.ima' trusted keyring" depends on IMA_TRUSTED_KEYRING diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index e2a60c3..9e82367 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -251,16 +251,4 @@ static inline int security_filter_rule_match(u3
[PATCHv3 4/6] evm: provide a function to set EVM key from the kernel
Crypto HW kernel module can possibly initialize EVM key from the kernel __init code to enable EVM before calling 'init' process. This patch provide a function evm_set_key() which can be used to set custom key directly to EVM without using KEY subsystem. Changes in v3: * error reporting moved to evm_set_key * EVM_INIT_HMAC moved to evm_set_key * added bitop to prevent key setting race Changes in v2: * use size_t for key size instead of signed int * provide EVM_MAX_KEY_SIZE macro in * provide EVM_MIN_KEY_SIZE macro in Signed-off-by: Dmitry Kasatkin --- include/linux/evm.h | 7 ++ security/integrity/evm/evm_crypto.c | 47 +++-- security/integrity/evm/evm_secfs.c | 10 +++- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/include/linux/evm.h b/include/linux/evm.h index 1fcb88c..35ed9a8 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -14,6 +14,7 @@ struct integrity_iint_cache; #ifdef CONFIG_EVM +extern int evm_set_key(void *key, size_t keylen); extern enum integrity_status evm_verifyxattr(struct dentry *dentry, const char *xattr_name, void *xattr_value, @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname) } #endif #else + +static inline int evm_set_key(void *key, size_t keylen) +{ + return -EOPNOTSUPP; +} + #ifdef CONFIG_INTEGRITY static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, const char *xattr_name, diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 34e1a6f..7aec93e 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include "evm.h" @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm; static DEFINE_MUTEX(mutex); +#define EVM_SET_KEY_BUSY 0 + +static unsigned long evm_set_key_flags; + +/* evm_set_key - set EVM HMAC key from the kernel + * + * This function allows to set EVM HMAC key from the kernel + * without using key subsystem 'encrypted' keys. It can be used + * by the crypto HW kernel module which has own way of managing + * keys. + * + * key length should be between 32 and 128 bytes long + */ +int evm_set_key(void *key, size_t keylen) +{ + int rc; + + rc = -EBUSY; + if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) + goto busy; + rc = -EINVAL; + if (keylen > MAX_KEY_SIZE) + goto inval; + memcpy(evmkey, key, keylen); + evm_initialized |= EVM_INIT_HMAC; + pr_info("key initialized\n"); + return 0; +inval: + clear_bit(EVM_SET_KEY_BUSY, _set_key_flags); +busy: + pr_err("key initialization failed\n"); + return rc; +} +EXPORT_SYMBOL_GPL(evm_set_key); + static struct shash_desc *init_desc(char type) { long rc; @@ -242,7 +278,7 @@ int evm_init_key(void) { struct key *evm_key; struct encrypted_key_payload *ekp; - int rc = 0; + int rc; evm_key = request_key(_type_encrypted, EVMKEY, NULL); if (IS_ERR(evm_key)) @@ -250,12 +286,9 @@ int evm_init_key(void) down_read(_key->sem); ekp = evm_key->payload.data; - if (ekp->decrypted_datalen > MAX_KEY_SIZE) { - rc = -EINVAL; - goto out; - } - memcpy(evmkey, ekp->decrypted_data, ekp->decrypted_datalen); -out: + + rc = evm_set_key(ekp->decrypted_data, ekp->decrypted_datalen); + /* burn the original key contents */ memset(ekp->decrypted_data, 0, ekp->decrypted_datalen); up_read(_key->sem); diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index 3f775df..c8dccd5 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -62,7 +62,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { char temp[80]; - int i, error; + int i; if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) return -EPERM; @@ -78,12 +78,8 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, if ((sscanf(temp, "%d", ) != 1) || (i != 1)) return -EINVAL; - error = evm_init_key(); - if (!error) { - evm_initialized |= EVM_INIT_HMAC; - pr_info("initialized\n"); - } else - pr_err("initialization failed\n"); + evm_init_key(); + return count; } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@v
[PATCHv3 0/6] integrity: few EVM patches
Hi, IMA module provides functionality to load x509 certificates into the trusted '.ima' keyring. This is patchset adds the same functionality to the EVM as well. Also it provides functionality to set EVM key from the kernel crypto HW driver. This is an update for the patchset which was previously sent for review few months ago. Please refer to the patch descriptions for details. BR, Dmitry Dmitry Kasatkin (6): integrity: define '.evm' as a builtin 'trusted' keyring evm: load x509 certificate from the kernel evm: enable EVM when X509 certificate is loaded evm: provide a function to set EVM key from the kernel evm: define EVM key max and min sizes evm: reset EVM status when file attributes changes include/linux/evm.h | 10 +++ security/integrity/Kconfig | 11 security/integrity/digsig.c | 14 -- security/integrity/evm/Kconfig | 17 security/integrity/evm/evm.h| 3 +++ security/integrity/evm/evm_crypto.c | 54 ++--- security/integrity/evm/evm_main.c | 32 +++--- security/integrity/evm/evm_secfs.c | 12 +++-- security/integrity/iint.c | 1 + security/integrity/ima/Kconfig | 5 +++- security/integrity/ima/ima.h| 12 - security/integrity/ima/ima_init.c | 2 +- security/integrity/integrity.h | 13 ++--- 13 files changed, 146 insertions(+), 40 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 2/6] evm: load x509 certificate from the kernel
This patch defines configuration option and the evm_load_x509() hook to load X509 certificate into the EVM trusted kernel keyring. Changes in v4: * Patch description updated Changes in v3: * Removed EVM_X509_PATH definition. CONFIG_EVM_X509_PATH is used directly. Changes in v2: * default key patch changed to /etc/keys Signed-off-by: Dmitry Kasatkin --- security/integrity/evm/Kconfig| 17 + security/integrity/evm/evm_main.c | 7 +++ security/integrity/iint.c | 1 + security/integrity/integrity.h| 8 4 files changed, 33 insertions(+) diff --git a/security/integrity/evm/Kconfig b/security/integrity/evm/Kconfig index bf19723..b1433e9 100644 --- a/security/integrity/evm/Kconfig +++ b/security/integrity/evm/Kconfig @@ -42,3 +42,20 @@ config EVM_EXTRA_SMACK_XATTRS additional info to the calculation, requires existing EVM labeled file systems to be relabeled. +config EVM_LOAD_X509 + bool "Load X509 certificate to the '.evm' trusted keyring" + depends on INTEGRITY_TRUSTED_KEYRING + default n + help + Load X509 certificate to the '.evm' trusted keyring. + + This option enables X509 certificate loading from the kernel + to the '.evm' trusted keyring. Public key can be used to + verify EVM integrity starting from 'init' process. + +config EVM_X509_PATH + string "EVM X509 certificate path" + depends on EVM_LOAD_X509 + default "/etc/keys/x509_evm.der" + help + This option defines X509 certificate path. diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 75b7e30..519de0a 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -472,6 +472,13 @@ out: } EXPORT_SYMBOL_GPL(evm_inode_init_security); +#ifdef CONFIG_EVM_LOAD_X509 +void __init evm_load_x509(void) +{ + integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); +} +#endif + static int __init init_evm(void) { int error; diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 3d2f5b4..2de9c82 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -254,4 +254,5 @@ out: void __init integrity_load_keys(void) { ima_load_x509(); + evm_load_x509(); } diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 07726a7..5efe2ec 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -170,6 +170,14 @@ static inline void ima_load_x509(void) } #endif +#ifdef CONFIG_EVM_LOAD_X509 +void __init evm_load_x509(void); +#else +static inline void evm_load_x509(void) +{ +} +#endif + #ifdef CONFIG_INTEGRITY_AUDIT /* declarations */ void integrity_audit_msg(int audit_msgno, struct inode *inode, -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 0/6] integrity: few EVM patches
Hi, IMA module provides functionality to load x509 certificates into the trusted '.ima' keyring. This is patchset adds the same functionality to the EVM as well. Also it provides functionality to set EVM key from the kernel crypto HW driver. This is an update for the patchset which was previously sent for review few months ago. Please refer to the patch descriptions for details. BR, Dmitry Dmitry Kasatkin (6): integrity: define '.evm' as a builtin 'trusted' keyring evm: load x509 certificate from the kernel evm: enable EVM when X509 certificate is loaded evm: provide a function to set EVM key from the kernel evm: define EVM key max and min sizes evm: reset EVM status when file attributes changes include/linux/evm.h | 10 +++ security/integrity/Kconfig | 11 security/integrity/digsig.c | 14 -- security/integrity/evm/Kconfig | 17 security/integrity/evm/evm.h| 3 +++ security/integrity/evm/evm_crypto.c | 54 ++--- security/integrity/evm/evm_main.c | 32 +++--- security/integrity/evm/evm_secfs.c | 12 +++-- security/integrity/iint.c | 1 + security/integrity/ima/Kconfig | 5 +++- security/integrity/ima/ima.h| 12 - security/integrity/ima/ima_init.c | 2 +- security/integrity/integrity.h | 13 ++--- 13 files changed, 146 insertions(+), 40 deletions(-) -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 4/6] evm: provide a function to set EVM key from the kernel
Crypto HW kernel module can possibly initialize EVM key from the kernel __init code to enable EVM before calling 'init' process. This patch provide a function evm_set_key() which can be used to set custom key directly to EVM without using KEY subsystem. Changes in v3: * error reporting moved to evm_set_key * EVM_INIT_HMAC moved to evm_set_key * added bitop to prevent key setting race Changes in v2: * use size_t for key size instead of signed int * provide EVM_MAX_KEY_SIZE macro in * provide EVM_MIN_KEY_SIZE macro in Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- include/linux/evm.h | 7 ++ security/integrity/evm/evm_crypto.c | 47 +++-- security/integrity/evm/evm_secfs.c | 10 +++- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/include/linux/evm.h b/include/linux/evm.h index 1fcb88c..35ed9a8 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -14,6 +14,7 @@ struct integrity_iint_cache; #ifdef CONFIG_EVM +extern int evm_set_key(void *key, size_t keylen); extern enum integrity_status evm_verifyxattr(struct dentry *dentry, const char *xattr_name, void *xattr_value, @@ -42,6 +43,12 @@ static inline int posix_xattr_acl(const char *xattrname) } #endif #else + +static inline int evm_set_key(void *key, size_t keylen) +{ + return -EOPNOTSUPP; +} + #ifdef CONFIG_INTEGRITY static inline enum integrity_status evm_verifyxattr(struct dentry *dentry, const char *xattr_name, diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 34e1a6f..7aec93e 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include "evm.h" @@ -32,6 +33,41 @@ struct crypto_shash *hash_tfm; static DEFINE_MUTEX(mutex); +#define EVM_SET_KEY_BUSY 0 + +static unsigned long evm_set_key_flags; + +/* evm_set_key - set EVM HMAC key from the kernel + * + * This function allows to set EVM HMAC key from the kernel + * without using key subsystem 'encrypted' keys. It can be used + * by the crypto HW kernel module which has own way of managing + * keys. + * + * key length should be between 32 and 128 bytes long + */ +int evm_set_key(void *key, size_t keylen) +{ + int rc; + + rc = -EBUSY; + if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) + goto busy; + rc = -EINVAL; + if (keylen > MAX_KEY_SIZE) + goto inval; + memcpy(evmkey, key, keylen); + evm_initialized |= EVM_INIT_HMAC; + pr_info("key initialized\n"); + return 0; +inval: + clear_bit(EVM_SET_KEY_BUSY, _set_key_flags); +busy: + pr_err("key initialization failed\n"); + return rc; +} +EXPORT_SYMBOL_GPL(evm_set_key); + static struct shash_desc *init_desc(char type) { long rc; @@ -242,7 +278,7 @@ int evm_init_key(void) { struct key *evm_key; struct encrypted_key_payload *ekp; - int rc = 0; + int rc; evm_key = request_key(_type_encrypted, EVMKEY, NULL); if (IS_ERR(evm_key)) @@ -250,12 +286,9 @@ int evm_init_key(void) down_read(_key->sem); ekp = evm_key->payload.data; - if (ekp->decrypted_datalen > MAX_KEY_SIZE) { - rc = -EINVAL; - goto out; - } - memcpy(evmkey, ekp->decrypted_data, ekp->decrypted_datalen); -out: + + rc = evm_set_key(ekp->decrypted_data, ekp->decrypted_datalen); + /* burn the original key contents */ memset(ekp->decrypted_data, 0, ekp->decrypted_datalen); up_read(_key->sem); diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index 3f775df..c8dccd5 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -62,7 +62,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { char temp[80]; - int i, error; + int i; if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) return -EPERM; @@ -78,12 +78,8 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, if ((sscanf(temp, "%d", ) != 1) || (i != 1)) return -EINVAL; - error = evm_init_key(); - if (!error) { - evm_initialized |= EVM_INIT_HMAC; - pr_info("initialized\n"); - } else - pr_err("initialization failed\n"); + evm_init_key(); + return count; } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in t
[PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring
Require all keys added to the EVM keyring be signed by an existing trusted key on the system trusted keyring. This patch also switches IMA to use integrity_init_keyring(). Changes in v3: * Added 'init_keyring' config based variable to skip initializing keyring instead of using __integrity_init_keyring() wrapper. * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING Changes in v2: * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config file compatibility. (Mimi Zohar) Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/Kconfig| 11 +++ security/integrity/digsig.c | 14 -- security/integrity/evm/evm_main.c | 8 +--- security/integrity/ima/Kconfig| 5 - security/integrity/ima/ima.h | 12 security/integrity/ima/ima_init.c | 2 +- security/integrity/integrity.h| 5 ++--- 7 files changed, 35 insertions(+), 22 deletions(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index 73c457b..21d7568 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS This option enables digital signature verification using asymmetric keys. +config INTEGRITY_TRUSTED_KEYRING + bool "Require all keys on the integrity keyrings be signed" + depends on SYSTEM_TRUSTED_KEYRING + depends on INTEGRITY_ASYMMETRIC_KEYS + select KEYS_DEBUG_PROC_KEYS + default y + help + This option requires that all keys added to the .ima and + .evm keyrings be signed by a key on the system trusted + keyring. + config INTEGRITY_AUDIT bool "Enables integrity auditing support " depends on AUDIT diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 5be9ffb..8ef1511 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -24,15 +24,22 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX]; static const char *keyring_name[INTEGRITY_KEYRING_MAX] = { +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING "_evm", - "_module", -#ifndef CONFIG_IMA_TRUSTED_KEYRING "_ima", #else + ".evm", ".ima", #endif + "_module", }; +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING +static bool init_keyring __initdata = true; +#else +static bool init_keyring __initdata; +#endif + int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, const char *digest, int digestlen) { @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id) const struct cred *cred = current_cred(); int err = 0; + if (!init_keyring) + return 0; + keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0), KGIDT_INIT(0), cred, ((KEY_POS_ALL & ~KEY_POS_SETATTR) | diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 1334e02..75b7e30 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -478,15 +478,17 @@ static int __init init_evm(void) evm_init_config(); + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM); + if (error) + return error; + error = evm_init_secfs(); if (error < 0) { pr_info("Error registering secfs\n"); - goto err; + return error; } return 0; -err: - return error; } /* diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index df30334..a292b88 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -123,14 +123,17 @@ config IMA_APPRAISE If unsure, say N. config IMA_TRUSTED_KEYRING - bool "Require all keys on the .ima keyring be signed" + bool "Require all keys on the .ima keyring be signed (deprecated)" depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING depends on INTEGRITY_ASYMMETRIC_KEYS + select INTEGRITY_TRUSTED_KEYRING default y help This option requires that all keys added to the .ima keyring be signed by a key on the system trusted keyring. + This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING + config IMA_LOAD_X509 bool "Load X509 certificate onto the '.ima' trusted keyring" depends on IMA_TRUSTED_KEYRING diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index e2a60c3..9e82367 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -251,16 +251,4 @@ static
[PATCHv3 3/6] evm: enable EVM when X509 certificate is loaded
In order to enable EVM before starting 'init' process, evm_initialized needs to be non-zero. Before it was indicating that HMAC key is loaded. When EVM loads X509 before calling 'init', it is possible to enable EVM to start signature based verification. This patch defines bits to enable EVM if key of any type is loaded. Changes in v2: * EVM_STATE_KEY_SET replaced by EVM_INIT_HMAC * EVM_STATE_X509_SET replaced by EVM_INIT_X509 Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/evm/evm.h| 3 +++ security/integrity/evm/evm_crypto.c | 2 ++ security/integrity/evm/evm_main.c | 6 +- security/integrity/evm/evm_secfs.c | 4 ++-- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index 88bfe77..f5f1272 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -21,6 +21,9 @@ #include "../integrity.h" +#define EVM_INIT_HMAC 0x0001 +#define EVM_INIT_X509 0x0002 + extern int evm_initialized; extern char *evm_hmac; extern char *evm_hash; diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 159ef3e..34e1a6f 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -40,6 +40,8 @@ static struct shash_desc *init_desc(char type) struct shash_desc *desc; if (type == EVM_XATTR_HMAC) { + if (!(evm_initialized & EVM_INIT_HMAC)) + return ERR_PTR(-ENOKEY); tfm = _tfm; algo = evm_hmac; } else { diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 519de0a..420d94d 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -475,7 +475,11 @@ EXPORT_SYMBOL_GPL(evm_inode_init_security); #ifdef CONFIG_EVM_LOAD_X509 void __init evm_load_x509(void) { - integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); + int rc; + + rc = integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); + if (!rc) + evm_initialized |= EVM_INIT_X509; } #endif diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c index cf12a04..3f775df 100644 --- a/security/integrity/evm/evm_secfs.c +++ b/security/integrity/evm/evm_secfs.c @@ -64,7 +64,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, char temp[80]; int i, error; - if (!capable(CAP_SYS_ADMIN) || evm_initialized) + if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_INIT_HMAC)) return -EPERM; if (count >= sizeof(temp) || count == 0) @@ -80,7 +80,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf, error = evm_init_key(); if (!error) { - evm_initialized = 1; + evm_initialized |= EVM_INIT_HMAC; pr_info("initialized\n"); } else pr_err("initialization failed\n"); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 6/6] evm: reset EVM status when file attributes changes
EVM verification status is cached in iint->evm_status and if it was successful, never re-verified again when IMA passes 'iint' to evm_verifyxattr(). When file attribute or extended attributes changes we may wish to re-verify EVM integrity as well. For example, after setting digital signature we may need to re-verify the signature and update iint->flags that there is EVM signature. This patch enables that by resetting evm_status to INTEGRITY_UKNOWN state. Changes in v2: * Flag setting moved to EVM layer Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/evm/evm_main.c | 13 + 1 file changed, 13 insertions(+) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 420d94d..f716025 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -358,6 +358,15 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name) return evm_protect_xattr(dentry, xattr_name, NULL, 0); } +static void evm_reset_status(struct inode *inode) +{ + struct integrity_iint_cache *iint; + + iint = integrity_iint_find(inode); + if (iint) + iint->evm_status = INTEGRITY_UNKNOWN; +} + /** * evm_inode_post_setxattr - update 'security.evm' to reflect the changes * @dentry: pointer to the affected dentry @@ -378,6 +387,8 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, && !posix_xattr_acl(xattr_name))) return; + evm_reset_status(dentry->d_inode); + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); } @@ -396,6 +407,8 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) if (!evm_initialized || !evm_protected_xattr(xattr_name)) return; + evm_reset_status(dentry->d_inode); + evm_update_evmxattr(dentry, xattr_name, NULL, 0); } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 5/6] evm: define EVM key max and min sizes
This patch imposes minimum key size limit. It declares EVM_MIN_KEY_SIZE and EVM_MAX_KEY_SIZE in public header file. Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- include/linux/evm.h | 3 +++ security/integrity/evm/evm_crypto.c | 7 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/linux/evm.h b/include/linux/evm.h index 35ed9a8..0aeedec 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -11,6 +11,9 @@ #include #include +#define EVM_MAX_KEY_SIZE 128 +#define EVM_MIN_KEY_SIZE 64 + struct integrity_iint_cache; #ifdef CONFIG_EVM diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 7aec93e..cfb788c 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -24,9 +24,8 @@ #include "evm.h" #define EVMKEY "evm-key" -#define MAX_KEY_SIZE 128 -static unsigned char evmkey[MAX_KEY_SIZE]; -static int evmkey_len = MAX_KEY_SIZE; +static unsigned char evmkey[EVM_MAX_KEY_SIZE]; +static int evmkey_len = EVM_MAX_KEY_SIZE; struct crypto_shash *hmac_tfm; struct crypto_shash *hash_tfm; @@ -54,7 +53,7 @@ int evm_set_key(void *key, size_t keylen) if (test_and_set_bit(EVM_SET_KEY_BUSY, _set_key_flags)) goto busy; rc = -EINVAL; - if (keylen > MAX_KEY_SIZE) + if (keylen < EVM_MIN_KEY_SIZE || keylen > EVM_MAX_KEY_SIZE) goto inval; memcpy(evmkey, key, keylen); evm_initialized |= EVM_INIT_HMAC; -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv3 2/6] evm: load x509 certificate from the kernel
This patch defines configuration option and the evm_load_x509() hook to load X509 certificate into the EVM trusted kernel keyring. Changes in v4: * Patch description updated Changes in v3: * Removed EVM_X509_PATH definition. CONFIG_EVM_X509_PATH is used directly. Changes in v2: * default key patch changed to /etc/keys Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> --- security/integrity/evm/Kconfig| 17 + security/integrity/evm/evm_main.c | 7 +++ security/integrity/iint.c | 1 + security/integrity/integrity.h| 8 4 files changed, 33 insertions(+) diff --git a/security/integrity/evm/Kconfig b/security/integrity/evm/Kconfig index bf19723..b1433e9 100644 --- a/security/integrity/evm/Kconfig +++ b/security/integrity/evm/Kconfig @@ -42,3 +42,20 @@ config EVM_EXTRA_SMACK_XATTRS additional info to the calculation, requires existing EVM labeled file systems to be relabeled. +config EVM_LOAD_X509 + bool "Load X509 certificate to the '.evm' trusted keyring" + depends on INTEGRITY_TRUSTED_KEYRING + default n + help + Load X509 certificate to the '.evm' trusted keyring. + + This option enables X509 certificate loading from the kernel + to the '.evm' trusted keyring. Public key can be used to + verify EVM integrity starting from 'init' process. + +config EVM_X509_PATH + string "EVM X509 certificate path" + depends on EVM_LOAD_X509 + default "/etc/keys/x509_evm.der" + help + This option defines X509 certificate path. diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 75b7e30..519de0a 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -472,6 +472,13 @@ out: } EXPORT_SYMBOL_GPL(evm_inode_init_security); +#ifdef CONFIG_EVM_LOAD_X509 +void __init evm_load_x509(void) +{ + integrity_load_x509(INTEGRITY_KEYRING_EVM, CONFIG_EVM_X509_PATH); +} +#endif + static int __init init_evm(void) { int error; diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 3d2f5b4..2de9c82 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -254,4 +254,5 @@ out: void __init integrity_load_keys(void) { ima_load_x509(); + evm_load_x509(); } diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 07726a7..5efe2ec 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -170,6 +170,14 @@ static inline void ima_load_x509(void) } #endif +#ifdef CONFIG_EVM_LOAD_X509 +void __init evm_load_x509(void); +#else +static inline void evm_load_x509(void) +{ +} +#endif + #ifdef CONFIG_INTEGRITY_AUDIT /* declarations */ void integrity_audit_msg(int audit_msgno, struct inode *inode, -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] integrity: prevent loading untrusted certificates to IMA trusted keyring
Hi, Apply this patch, please... Dmitry On Thu, Sep 10, 2015 at 10:06 PM, Dmitry Kasatkin wrote: > If IMA_LOAD_X509 is enabled either directly or indirectly via > IMA_APPRAISE_SIGNED_INIT, it enables certificate loading to the IMA trusted > keyring from the kernel. Due to the overlook, KEY_ALLOC_TRUSTED was used in > the > key_create_or_update() to create keys within the kernel, which caused > overriding certificate verification result and allowed to load self-signed or > wrongly signed certificates. > > This patch just removes this option. > > Signed-off-by: Dmitry Kasatkin > Cc: # 3.19+ > --- > security/integrity/digsig.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index 36fb6b5..5be9ffb 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -105,7 +105,7 @@ int __init integrity_load_x509(const unsigned int id, > const char *path) >rc, >((KEY_POS_ALL & ~KEY_POS_SETATTR) | > KEY_USR_VIEW | KEY_USR_READ), > - KEY_ALLOC_NOT_IN_QUOTA | > KEY_ALLOC_TRUSTED); > + KEY_ALLOC_NOT_IN_QUOTA); > if (IS_ERR(key)) { > rc = PTR_ERR(key); > pr_err("Problem loading X.509 certificate (%d): %s\n", > -- > 2.1.4 > -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] integrity: prevent loading untrusted certificates to IMA trusted keyring
Hi, Apply this patch, please... Dmitry On Thu, Sep 10, 2015 at 10:06 PM, Dmitry Kasatkin <dmitry.kasat...@gmail.com> wrote: > If IMA_LOAD_X509 is enabled either directly or indirectly via > IMA_APPRAISE_SIGNED_INIT, it enables certificate loading to the IMA trusted > keyring from the kernel. Due to the overlook, KEY_ALLOC_TRUSTED was used in > the > key_create_or_update() to create keys within the kernel, which caused > overriding certificate verification result and allowed to load self-signed or > wrongly signed certificates. > > This patch just removes this option. > > Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> > Cc: <sta...@vger.kernel.org> # 3.19+ > --- > security/integrity/digsig.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c > index 36fb6b5..5be9ffb 100644 > --- a/security/integrity/digsig.c > +++ b/security/integrity/digsig.c > @@ -105,7 +105,7 @@ int __init integrity_load_x509(const unsigned int id, > const char *path) >rc, >((KEY_POS_ALL & ~KEY_POS_SETATTR) | > KEY_USR_VIEW | KEY_USR_READ), > - KEY_ALLOC_NOT_IN_QUOTA | > KEY_ALLOC_TRUSTED); > + KEY_ALLOC_NOT_IN_QUOTA); > if (IS_ERR(key)) { > rc = PTR_ERR(key); > pr_err("Problem loading X.509 certificate (%d): %s\n", > -- > 2.1.4 > -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] integrity: prevent loading untrusted certificates to IMA trusted keyring
If IMA_LOAD_X509 is enabled either directly or indirectly via IMA_APPRAISE_SIGNED_INIT, it enables certificate loading to the IMA trusted keyring from the kernel. Due to the overlook, KEY_ALLOC_TRUSTED was used in the key_create_or_update() to create keys within the kernel, which caused overriding certificate verification result and allowed to load self-signed or wrongly signed certificates. This patch just removes this option. Signed-off-by: Dmitry Kasatkin Cc: # 3.19+ --- security/integrity/digsig.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 36fb6b5..5be9ffb 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -105,7 +105,7 @@ int __init integrity_load_x509(const unsigned int id, const char *path) rc, ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ), - KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_TRUSTED); + KEY_ALLOC_NOT_IN_QUOTA); if (IS_ERR(key)) { rc = PTR_ERR(key); pr_err("Problem loading X.509 certificate (%d): %s\n", -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] integrity: prevent loading untrusted certificates to IMA trusted keyring
If IMA_LOAD_X509 is enabled either directly or indirectly via IMA_APPRAISE_SIGNED_INIT, it enables certificate loading to the IMA trusted keyring from the kernel. Due to the overlook, KEY_ALLOC_TRUSTED was used in the key_create_or_update() to create keys within the kernel, which caused overriding certificate verification result and allowed to load self-signed or wrongly signed certificates. This patch just removes this option. Signed-off-by: Dmitry Kasatkin <dmitry.kasat...@huawei.com> Cc: <sta...@vger.kernel.org> # 3.19+ --- security/integrity/digsig.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 36fb6b5..5be9ffb 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -105,7 +105,7 @@ int __init integrity_load_x509(const unsigned int id, const char *path) rc, ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ), - KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_TRUSTED); + KEY_ALLOC_NOT_IN_QUOTA); if (IS_ERR(key)) { rc = PTR_ERR(key); pr_err("Problem loading X.509 certificate (%d): %s\n", -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: contact change for the integrity tree
Hi, Yes, please. (in plain text) - Dmitry On 26 January 2015 at 22:49, Stephen Rothwell wrote: > Hi all, > > I noticed commit bfd33c4b4b1a ("MAINTAINERS: email update") in the > integrity tree today. I assume that I should also update the email > address in my contacts list? > > -- > Cheers, > Stephen Rothwells...@canb.auug.org.au -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: contact change for the integrity tree
Hi, Yes, please. (in plain text) - Dmitry On 26 January 2015 at 22:49, Stephen Rothwell s...@canb.auug.org.au wrote: Hi all, I noticed commit bfd33c4b4b1a (MAINTAINERS: email update) in the integrity tree today. I assume that I should also update the email address in my contacts list? -- Cheers, Stephen Rothwells...@canb.auug.org.au -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/1] Email update
Hello, Sorry for the ugly typo in MAINTAINERS. - Dmitry Dmitry Kasatkin (1): MAINTAINERS: email update MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/1] MAINTAINERS: email update
Changed to my private email address as I left Samsung. Signed-off-by: Dmitry Kasatkin --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index ccb0fef..0ee6758 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4655,7 +4655,7 @@ F:drivers/ipack/ INTEGRITY MEASUREMENT ARCHITECTURE (IMA) M: Mimi Zohar -M: Dmitry Kasatkin +M: Dmitry Kasatkin L: linux-ima-de...@lists.sourceforge.net L: linux-ima-u...@lists.sourceforge.net L: linux-security-mod...@vger.kernel.org -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] MAINTEINERS: email update
Changed to my private email address as I left Samsung. Signed-off-by: Dmitry Kasatkin --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index ccb0fef..0ee6758 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4655,7 +4655,7 @@ F:drivers/ipack/ INTEGRITY MEASUREMENT ARCHITECTURE (IMA) M: Mimi Zohar -M: Dmitry Kasatkin +M: Dmitry Kasatkin L: linux-ima-de...@lists.sourceforge.net L: linux-ima-u...@lists.sourceforge.net L: linux-security-mod...@vger.kernel.org -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] MAINTEINERS: email update
Changed to my private email address as I left Samsung. Signed-off-by: Dmitry Kasatkin dmitry.kasat...@gmail.com --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index ccb0fef..0ee6758 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4655,7 +4655,7 @@ F:drivers/ipack/ INTEGRITY MEASUREMENT ARCHITECTURE (IMA) M: Mimi Zohar zo...@linux.vnet.ibm.com -M: Dmitry Kasatkin d.kasat...@samsung.com +M: Dmitry Kasatkin dmitry.kasat...@gmail.com L: linux-ima-de...@lists.sourceforge.net L: linux-ima-u...@lists.sourceforge.net L: linux-security-mod...@vger.kernel.org -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/1] MAINTAINERS: email update
Changed to my private email address as I left Samsung. Signed-off-by: Dmitry Kasatkin dmitry.kasat...@gmail.com --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index ccb0fef..0ee6758 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4655,7 +4655,7 @@ F:drivers/ipack/ INTEGRITY MEASUREMENT ARCHITECTURE (IMA) M: Mimi Zohar zo...@linux.vnet.ibm.com -M: Dmitry Kasatkin d.kasat...@samsung.com +M: Dmitry Kasatkin dmitry.kasat...@gmail.com L: linux-ima-de...@lists.sourceforge.net L: linux-ima-u...@lists.sourceforge.net L: linux-security-mod...@vger.kernel.org -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/1] Email update
Hello, Sorry for the ugly typo in MAINTAINERS. - Dmitry Dmitry Kasatkin (1): MAINTAINERS: email update MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 2/2] MPILIB: Deobfuscate mpi_cmp
correct. Acked-by: Dmitry Kasatkin Dmitry On 12 January 2015 at 13:43, David Howells wrote: > Dmitry Kasatkin wrote: > >> Ack. > > To what email address do I translate that now? > > Acked-by: Dmitry Kasatkin > > perchance? > > David -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 2/2] MPILIB: Deobfuscate mpi_cmp
correct. Acked-by: Dmitry Kasatkin dmitry.kasat...@gmail.com Dmitry On 12 January 2015 at 13:43, David Howells dhowe...@redhat.com wrote: Dmitry Kasatkin dmitry.kasat...@gmail.com wrote: Ack. To what email address do I translate that now? Acked-by: Dmitry Kasatkin dmitry.kasat...@gmail.com perchance? David -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 2/2] MPILIB: Deobfuscate mpi_cmp
Hi, Thank you. Indeed '-cmp' is much more clear. Ack. - Dmitry On 9 January 2015 at 13:00, David Howells wrote: > This looks very reasonable. cc'ing Dmitry for his check. > > David > --- > Rasmus Villemoes wrote: > >> The condition preceding 'return 1;' makes my head hurt. At this point, >> we know that u and v have the same sign; if they are negative, they >> compare opposite to how their absolute values compare (which >> mpihelp_cmp found for us), otherwise cmp itself is the >> answer. Negating cmp is ok since mpihelp_cmp returns {-1,0,1}; >> -INT_MIN==INT_MIN won't bite us. >> >> Signed-off-by: Rasmus Villemoes >> --- >> lib/mpi/mpi-cmp.c | 8 +++- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/lib/mpi/mpi-cmp.c b/lib/mpi/mpi-cmp.c >> index 3801694240d8..d25e9e96c310 100644 >> --- a/lib/mpi/mpi-cmp.c >> +++ b/lib/mpi/mpi-cmp.c >> @@ -61,10 +61,8 @@ int mpi_cmp(MPI u, MPI v) >> if (!usize) >> return 0; >> cmp = mpihelp_cmp(u->d, v->d, usize); >> - if (!cmp) >> - return 0; >> - if ((cmp < 0 ? 1 : 0) == (u->sign ? 1 : 0)) >> - return 1; >> - return -1; >> + if (u->sign) >> + return -cmp; >> + return cmp; >> } >> EXPORT_SYMBOL_GPL(mpi_cmp); >> -- >> 2.1.3 > ___ > Keyrings mailing list > keyri...@linux-nfs.org > To change your subscription to this list, please see > http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 1/2] MPILIB: Fix comparison of negative MPIs
Hi, Thank you. It looks correct. Ack. - Dmitry On 9 January 2015 at 12:58, David Howells wrote: > I think you're right - *adding* the two sizes makes no sense. cc'ing Dmitry > also for his check. > > David > > > Rasmus Villemoes wrote: > >> If u and v both represent negative integers and their limb counts >> happen to differ, mpi_cmp will always return a positive value - this >> is obviously bogus. u is smaller than v if and only if it is larger in >> absolute value. >> >> Signed-off-by: Rasmus Villemoes >> --- >> lib/mpi/mpi-cmp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/mpi/mpi-cmp.c b/lib/mpi/mpi-cmp.c >> index 1871e7b61ca0..3801694240d8 100644 >> --- a/lib/mpi/mpi-cmp.c >> +++ b/lib/mpi/mpi-cmp.c >> @@ -57,7 +57,7 @@ int mpi_cmp(MPI u, MPI v) >> if (usize != vsize && !u->sign && !v->sign) >> return usize - vsize; >> if (usize != vsize && u->sign && v->sign) >> - return vsize + usize; >> + return vsize - usize; >> if (!usize) >> return 0; >> cmp = mpihelp_cmp(u->d, v->d, usize); >> -- >> 2.1.3 >> > ___ > Keyrings mailing list > keyri...@linux-nfs.org > To change your subscription to this list, please see > http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 2/2] MPILIB: Deobfuscate mpi_cmp
Hi, Thank you. Indeed '-cmp' is much more clear. Ack. - Dmitry On 9 January 2015 at 13:00, David Howells dhowe...@redhat.com wrote: This looks very reasonable. cc'ing Dmitry for his check. David --- Rasmus Villemoes li...@rasmusvillemoes.dk wrote: The condition preceding 'return 1;' makes my head hurt. At this point, we know that u and v have the same sign; if they are negative, they compare opposite to how their absolute values compare (which mpihelp_cmp found for us), otherwise cmp itself is the answer. Negating cmp is ok since mpihelp_cmp returns {-1,0,1}; -INT_MIN==INT_MIN won't bite us. Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk --- lib/mpi/mpi-cmp.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/mpi/mpi-cmp.c b/lib/mpi/mpi-cmp.c index 3801694240d8..d25e9e96c310 100644 --- a/lib/mpi/mpi-cmp.c +++ b/lib/mpi/mpi-cmp.c @@ -61,10 +61,8 @@ int mpi_cmp(MPI u, MPI v) if (!usize) return 0; cmp = mpihelp_cmp(u-d, v-d, usize); - if (!cmp) - return 0; - if ((cmp 0 ? 1 : 0) == (u-sign ? 1 : 0)) - return 1; - return -1; + if (u-sign) + return -cmp; + return cmp; } EXPORT_SYMBOL_GPL(mpi_cmp); -- 2.1.3 ___ Keyrings mailing list keyri...@linux-nfs.org To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Keyrings] [PATCH 1/2] MPILIB: Fix comparison of negative MPIs
Hi, Thank you. It looks correct. Ack. - Dmitry On 9 January 2015 at 12:58, David Howells dhowe...@redhat.com wrote: I think you're right - *adding* the two sizes makes no sense. cc'ing Dmitry also for his check. David Rasmus Villemoes li...@rasmusvillemoes.dk wrote: If u and v both represent negative integers and their limb counts happen to differ, mpi_cmp will always return a positive value - this is obviously bogus. u is smaller than v if and only if it is larger in absolute value. Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk --- lib/mpi/mpi-cmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mpi/mpi-cmp.c b/lib/mpi/mpi-cmp.c index 1871e7b61ca0..3801694240d8 100644 --- a/lib/mpi/mpi-cmp.c +++ b/lib/mpi/mpi-cmp.c @@ -57,7 +57,7 @@ int mpi_cmp(MPI u, MPI v) if (usize != vsize !u-sign !v-sign) return usize - vsize; if (usize != vsize u-sign v-sign) - return vsize + usize; + return vsize - usize; if (!usize) return 0; cmp = mpihelp_cmp(u-d, v-d, usize); -- 2.1.3 ___ Keyrings mailing list keyri...@linux-nfs.org To change your subscription to this list, please see http://linux-nfs.org/cgi-bin/mailman/listinfo/keyrings -- Thanks, Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
On 05/12/14 16:04, David Howells wrote: > Dmitry Kasatkin wrote: > >> With just "make all" on Ubuntu. > What gcc? I don't see any warnings. > > David > $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.8.2-19ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
On 05/12/14 12:23, David Howells wrote: > Dmitry Kasatkin wrote: > >> sign-file.c produce lots of annoying noise. > How did you get it to produce that? > > David > With just "make all" on Ubuntu. - Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
Hi David, sign-file.c produce lots of annoying noise. scripts/sign-file.c:153:2: warning: format not a string literal and no format arguments [-Wformat-security] ERR(!bd, dest_name); ^ scripts/sign-file.c:179:3: warning: format not a string literal and no format arguments [-Wformat-security] ERR(!b, pkcs7_name); ^ scripts/sign-file.c:180:3: warning: format not a string literal and no format arguments [-Wformat-security] ERR(i2d_PKCS7_bio_stream(b, pkcs7, NULL, 0) < 0, pkcs7_name); ^ scripts/sign-file.c:185:2: warning: format not a string literal and no format arguments [-Wformat-security] ERR(BIO_reset(bm) < 0, module_name); ^ Would be great to fix it. Thanks, Dmitry On 26/11/14 16:17, David Howells wrote: > Provide a utility that: > > (1) Digests a module using the specified hash algorithm (typically sha256). > > [The digest can be dumped into a file by passing the '-d' flag] > > (2) Generates a PKCS#7 message that: > > (a) Has detached data (ie. the module content). > > (b) Is signed with the specified private key. > > (c) Refers to the specified X.509 certificate. > > (d) Has an empty X.509 certificate list. > > [The PKCS#7 message can be dumped into a file by passing the '-p' flag] > > (3) Generates a signed module by concatenating the old module, the PKCS#7 > message, a descriptor and a magic string. The descriptor contains the > size of the PKCS#7 message and indicates the id_type as PKEY_ID_PKCS7. > > (4) Either writes the signed module to the specified destination or renames > it over the source module. > > This allows module signing to reuse the PKCS#7 handling code that was added > for PE file parsing for signed kexec. > > Note that the utility is written in C and must be linked against the OpenSSL > crypto library. > > Note further that I have temporarily dropped support for handling externally > created signatures until we can work out the best way to do those. Hopefully, > whoever creates the signature can give me a PKCS#7 certificate. > > Signed-off-by: David Howells > --- > > include/crypto/public_key.h |1 > scripts/sign-file.c | 189 > +++ > 2 files changed, 190 insertions(+) > create mode 100755 scripts/sign-file.c > > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h > index b6f27a240856..fda097e079a4 100644 > --- a/include/crypto/public_key.h > +++ b/include/crypto/public_key.h > @@ -33,6 +33,7 @@ extern const struct public_key_algorithm > *pkey_algo[PKEY_ALGO__LAST]; > enum pkey_id_type { > PKEY_ID_PGP,/* OpenPGP generated key ID */ > PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */ > + PKEY_ID_PKCS7, /* Signature in PKCS#7 message */ > PKEY_ID_TYPE__LAST > }; > > diff --git a/scripts/sign-file.c b/scripts/sign-file.c > new file mode 100755 > index ..3f9bedbd185f > --- /dev/null > +++ b/scripts/sign-file.c > @@ -0,0 +1,189 @@ > +/* Sign a module file using the given key. > + * > + * Copyright (C) 2014 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowe...@redhat.com) > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public Licence > + * as published by the Free Software Foundation; either version > + * 2 of the Licence, or (at your option) any later version. > + */ > +#define _GNU_SOURCE > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +struct module_signature { > + uint8_t algo; /* Public-key crypto algorithm [0] */ > + uint8_t hash; /* Digest algorithm [0] */ > + uint8_t id_type;/* Key identifier type [PKEY_ID_PKCS7] > */ > + uint8_t signer_len; /* Length of signer's name [0] */ > + uint8_t key_id_len; /* Length of key identifier [0] */ > + uint8_t __pad[3]; > + uint32_tsig_len;/* Length of signature data */ > +}; > + > +#define PKEY_ID_PKCS7 2 > + > +static char magic_number[] = "~Module signature appended~\n"; > + > +static __attribute__((noreturn)) > +void format(void) > +{ > + fprintf(stderr, > + "Usage: scripts/sign-file [-dp] > []\n"); > + exit(2); > +} > + > +static void display_openssl_errors(int l) > +{ > + const char *file; > + char buf[120]; > + int e, line; > + > + if (ERR_peek_error() == 0) > + return; > + fprintf(stderr, "At main.c:%d:\n", l); > + > + while ((e = ERR_get_error_line(, ))) { > + ERR_error_string(e, buf); > + fprintf(stderr, "- SSL %s: %s:%d\n", buf, file, line); > + } > +} > + > + > +#define ERR(cond, ...) \ > + do { \ >
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
Hi David, sign-file.c produce lots of annoying noise. scripts/sign-file.c:153:2: warning: format not a string literal and no format arguments [-Wformat-security] ERR(!bd, dest_name); ^ scripts/sign-file.c:179:3: warning: format not a string literal and no format arguments [-Wformat-security] ERR(!b, pkcs7_name); ^ scripts/sign-file.c:180:3: warning: format not a string literal and no format arguments [-Wformat-security] ERR(i2d_PKCS7_bio_stream(b, pkcs7, NULL, 0) 0, pkcs7_name); ^ scripts/sign-file.c:185:2: warning: format not a string literal and no format arguments [-Wformat-security] ERR(BIO_reset(bm) 0, module_name); ^ Would be great to fix it. Thanks, Dmitry On 26/11/14 16:17, David Howells wrote: Provide a utility that: (1) Digests a module using the specified hash algorithm (typically sha256). [The digest can be dumped into a file by passing the '-d' flag] (2) Generates a PKCS#7 message that: (a) Has detached data (ie. the module content). (b) Is signed with the specified private key. (c) Refers to the specified X.509 certificate. (d) Has an empty X.509 certificate list. [The PKCS#7 message can be dumped into a file by passing the '-p' flag] (3) Generates a signed module by concatenating the old module, the PKCS#7 message, a descriptor and a magic string. The descriptor contains the size of the PKCS#7 message and indicates the id_type as PKEY_ID_PKCS7. (4) Either writes the signed module to the specified destination or renames it over the source module. This allows module signing to reuse the PKCS#7 handling code that was added for PE file parsing for signed kexec. Note that the utility is written in C and must be linked against the OpenSSL crypto library. Note further that I have temporarily dropped support for handling externally created signatures until we can work out the best way to do those. Hopefully, whoever creates the signature can give me a PKCS#7 certificate. Signed-off-by: David Howells dhowe...@redhat.com --- include/crypto/public_key.h |1 scripts/sign-file.c | 189 +++ 2 files changed, 190 insertions(+) create mode 100755 scripts/sign-file.c diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h index b6f27a240856..fda097e079a4 100644 --- a/include/crypto/public_key.h +++ b/include/crypto/public_key.h @@ -33,6 +33,7 @@ extern const struct public_key_algorithm *pkey_algo[PKEY_ALGO__LAST]; enum pkey_id_type { PKEY_ID_PGP,/* OpenPGP generated key ID */ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */ + PKEY_ID_PKCS7, /* Signature in PKCS#7 message */ PKEY_ID_TYPE__LAST }; diff --git a/scripts/sign-file.c b/scripts/sign-file.c new file mode 100755 index ..3f9bedbd185f --- /dev/null +++ b/scripts/sign-file.c @@ -0,0 +1,189 @@ +/* Sign a module file using the given key. + * + * Copyright (C) 2014 Red Hat, Inc. All Rights Reserved. + * Written by David Howells (dhowe...@redhat.com) + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public Licence + * as published by the Free Software Foundation; either version + * 2 of the Licence, or (at your option) any later version. + */ +#define _GNU_SOURCE +#include stdio.h +#include stdlib.h +#include stdint.h +#include stdbool.h +#include string.h +#include getopt.h +#include err.h +#include arpa/inet.h +#include openssl/bio.h +#include openssl/evp.h +#include openssl/pem.h +#include openssl/pkcs7.h +#include openssl/err.h + +struct module_signature { + uint8_t algo; /* Public-key crypto algorithm [0] */ + uint8_t hash; /* Digest algorithm [0] */ + uint8_t id_type;/* Key identifier type [PKEY_ID_PKCS7] */ + uint8_t signer_len; /* Length of signer's name [0] */ + uint8_t key_id_len; /* Length of key identifier [0] */ + uint8_t __pad[3]; + uint32_tsig_len;/* Length of signature data */ +}; + +#define PKEY_ID_PKCS7 2 + +static char magic_number[] = ~Module signature appended~\n; + +static __attribute__((noreturn)) +void format(void) +{ + fprintf(stderr, + Usage: scripts/sign-file [-dp] hash algo key x509 module [dest]\n); + exit(2); +} + +static void display_openssl_errors(int l) +{ + const char *file; + char buf[120]; + int e, line; + + if (ERR_peek_error() == 0) + return; + fprintf(stderr, At main.c:%d:\n, l); + + while ((e = ERR_get_error_line(file, line))) { + ERR_error_string(e, buf); + fprintf(stderr, - SSL %s: %s:%d\n, buf, file, line); + } +} + + +#define ERR(cond, ...) \ + do
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
On 05/12/14 12:23, David Howells wrote: Dmitry Kasatkin d.kasat...@samsung.com wrote: sign-file.c produce lots of annoying noise. How did you get it to produce that? David With just make all on Ubuntu. - Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] MODSIGN: Provide a utility to append a PKCS#7 signature to a module [ver #2]
On 05/12/14 16:04, David Howells wrote: Dmitry Kasatkin d.kasat...@samsung.com wrote: With just make all on Ubuntu. What gcc? I don't see any warnings. David $ gcc -v Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.8.2-19ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] X.509: Extract both parts of the AuthorityKeyIdentifier
On 21/11/14 16:42, Vivek Goyal wrote: > On Thu, Nov 20, 2014 at 04:54:03PM +, David Howells wrote: > > [..] >> diff --git a/crypto/asymmetric_keys/x509_parser.h >> b/crypto/asymmetric_keys/x509_parser.h >> index 3dfe6b5d6f0b..223b72344060 100644 >> --- a/crypto/asymmetric_keys/x509_parser.h >> +++ b/crypto/asymmetric_keys/x509_parser.h >> @@ -21,7 +21,8 @@ struct x509_certificate { >> char*subject; /* Name of certificate subject >> */ >> struct asymmetric_key_id *id; /* Serial number + issuer */ >> struct asymmetric_key_id *skid; /* Subject + subjectKeyId >> (optional) */ >> -struct asymmetric_key_id *authority;/* Authority key identifier >> (optional) */ >> +struct asymmetric_key_id *auth_id; /* CA AuthKeyId matching ->id >> (optional) */ >> +struct asymmetric_key_id *auth_skid;/* CA AuthKeyId matching ->skid >> (optional) */ > A very minor nit. It might help if we put additional comment to explain what > auth_id and auth_skid are composed of (like other key ids). > > auth_id /* akid issuer + akid serial */ > auth_skid /* issuer + akid keyid */ > > Thanks > Vivek > Right, David did not address this in his v2 patchset... - Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] X.509: Extract both parts of the AuthorityKeyIdentifier [ver #2]
On 26/11/14 16:17, David Howells wrote: > Extract both parts of the AuthorityKeyIdentifier, not just the keyIdentifier, > as the second part can be used to match X.509 certificates by issuer and > serialNumber. > > Signed-off-by: David Howells > --- > > crypto/asymmetric_keys/Makefile |8 +- > crypto/asymmetric_keys/pkcs7_trust.c |4 - > crypto/asymmetric_keys/pkcs7_verify.c | 12 +- > crypto/asymmetric_keys/x509_akid.asn1 | 35 +++ > crypto/asymmetric_keys/x509_cert_parser.c | 142 > ++--- > crypto/asymmetric_keys/x509_parser.h |5 + > crypto/asymmetric_keys/x509_public_key.c |8 +- > 7 files changed, 145 insertions(+), 69 deletions(-) > create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 > > diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile > index e47fcd9ac5e8..cd1406f9b14a 100644 > --- a/crypto/asymmetric_keys/Makefile > +++ b/crypto/asymmetric_keys/Makefile > @@ -15,15 +15,21 @@ obj-$(CONFIG_PUBLIC_KEY_ALGO_RSA) += rsa.o > obj-$(CONFIG_X509_CERTIFICATE_PARSER) += x509_key_parser.o > x509_key_parser-y := \ > x509-asn1.o \ > + x509_akid-asn1.o \ > x509_rsakey-asn1.o \ > x509_cert_parser.o \ > x509_public_key.o > > -$(obj)/x509_cert_parser.o: $(obj)/x509-asn1.h $(obj)/x509_rsakey-asn1.h > +$(obj)/x509_cert_parser.o: \ > + $(obj)/x509-asn1.h \ > + $(obj)/x509_akid-asn1.h \ > + $(obj)/x509_rsakey-asn1.h > $(obj)/x509-asn1.o: $(obj)/x509-asn1.c $(obj)/x509-asn1.h > +$(obj)/x509_akid-asn1.o: $(obj)/x509_akid-asn1.c $(obj)/x509_akid-asn1.h > $(obj)/x509_rsakey-asn1.o: $(obj)/x509_rsakey-asn1.c > $(obj)/x509_rsakey-asn1.h > > clean-files += x509-asn1.c x509-asn1.h > +clean-files += x509_akid-asn1.c x509_akid-asn1.h > clean-files += x509_rsakey-asn1.c x509_rsakey-asn1.h > > # > diff --git a/crypto/asymmetric_keys/pkcs7_trust.c > b/crypto/asymmetric_keys/pkcs7_trust.c > index 1d29376072da..f802cf118053 100644 > --- a/crypto/asymmetric_keys/pkcs7_trust.c > +++ b/crypto/asymmetric_keys/pkcs7_trust.c > @@ -85,8 +85,8 @@ static int pkcs7_validate_trust_one(struct pkcs7_message > *pkcs7, > /* No match - see if the root certificate has a signer amongst the >* trusted keys. >*/ > - if (last && last->authority) { > - key = x509_request_asymmetric_key(trust_keyring, > last->authority, > + if (last && last->auth_skid) { > + key = x509_request_asymmetric_key(trust_keyring, > last->auth_skid, > false); > if (!IS_ERR(key)) { > x509 = last; > diff --git a/crypto/asymmetric_keys/pkcs7_verify.c > b/crypto/asymmetric_keys/pkcs7_verify.c > index cd455450b069..5e956c5b9071 100644 > --- a/crypto/asymmetric_keys/pkcs7_verify.c > +++ b/crypto/asymmetric_keys/pkcs7_verify.c > @@ -187,11 +187,11 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message > *pkcs7, > goto maybe_missing_crypto_in_x509; > > pr_debug("- issuer %s\n", x509->issuer); > - if (x509->authority) > + if (x509->auth_skid) > pr_debug("- authkeyid %*phN\n", > - x509->authority->len, x509->authority->data); > + x509->auth_skid->len, x509->auth_skid->data); > > - if (!x509->authority || > + if (!x509->auth_skid || > strcmp(x509->subject, x509->issuer) == 0) { > /* If there's no authority certificate specified, then >* the certificate must be self-signed and is the root > @@ -216,13 +216,13 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message > *pkcs7, >* list to see if the next one is there. >*/ > pr_debug("- want %*phN\n", > - x509->authority->len, x509->authority->data); > + x509->auth_skid->len, x509->auth_skid->data); > for (p = pkcs7->certs; p; p = p->next) { > if (!p->skid) > continue; > pr_debug("- cmp [%u] %*phN\n", >p->index, p->skid->len, p->skid->data); > - if (asymmetric_key_id_same(p->skid, x509->authority)) > + if (asymmetric_key_id_same(p->skid, x509->auth_skid)) > goto found_issuer; > } > > @@ -338,8 +338,6 @@ int pkcs7_verify(struct pkcs7_message *pkcs7) > ret = x509_get_sig_params(x509); > if (ret < 0) > return ret; > - pr_debug("X.509[%u] %*phN\n", > - n, x509->authority->len, x509->authority->data); > } > > for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) { > diff --git a/crypto/asymmetric_keys/x509_akid.asn1
Re: [PATCH 1/5] X.509: Extract both parts of the AuthorityKeyIdentifier [ver #2]
On 26/11/14 16:17, David Howells wrote: Extract both parts of the AuthorityKeyIdentifier, not just the keyIdentifier, as the second part can be used to match X.509 certificates by issuer and serialNumber. Signed-off-by: David Howells dhowe...@redhat.com --- crypto/asymmetric_keys/Makefile |8 +- crypto/asymmetric_keys/pkcs7_trust.c |4 - crypto/asymmetric_keys/pkcs7_verify.c | 12 +- crypto/asymmetric_keys/x509_akid.asn1 | 35 +++ crypto/asymmetric_keys/x509_cert_parser.c | 142 ++--- crypto/asymmetric_keys/x509_parser.h |5 + crypto/asymmetric_keys/x509_public_key.c |8 +- 7 files changed, 145 insertions(+), 69 deletions(-) create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile index e47fcd9ac5e8..cd1406f9b14a 100644 --- a/crypto/asymmetric_keys/Makefile +++ b/crypto/asymmetric_keys/Makefile @@ -15,15 +15,21 @@ obj-$(CONFIG_PUBLIC_KEY_ALGO_RSA) += rsa.o obj-$(CONFIG_X509_CERTIFICATE_PARSER) += x509_key_parser.o x509_key_parser-y := \ x509-asn1.o \ + x509_akid-asn1.o \ x509_rsakey-asn1.o \ x509_cert_parser.o \ x509_public_key.o -$(obj)/x509_cert_parser.o: $(obj)/x509-asn1.h $(obj)/x509_rsakey-asn1.h +$(obj)/x509_cert_parser.o: \ + $(obj)/x509-asn1.h \ + $(obj)/x509_akid-asn1.h \ + $(obj)/x509_rsakey-asn1.h $(obj)/x509-asn1.o: $(obj)/x509-asn1.c $(obj)/x509-asn1.h +$(obj)/x509_akid-asn1.o: $(obj)/x509_akid-asn1.c $(obj)/x509_akid-asn1.h $(obj)/x509_rsakey-asn1.o: $(obj)/x509_rsakey-asn1.c $(obj)/x509_rsakey-asn1.h clean-files += x509-asn1.c x509-asn1.h +clean-files += x509_akid-asn1.c x509_akid-asn1.h clean-files += x509_rsakey-asn1.c x509_rsakey-asn1.h # diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c index 1d29376072da..f802cf118053 100644 --- a/crypto/asymmetric_keys/pkcs7_trust.c +++ b/crypto/asymmetric_keys/pkcs7_trust.c @@ -85,8 +85,8 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7, /* No match - see if the root certificate has a signer amongst the * trusted keys. */ - if (last last-authority) { - key = x509_request_asymmetric_key(trust_keyring, last-authority, + if (last last-auth_skid) { + key = x509_request_asymmetric_key(trust_keyring, last-auth_skid, false); if (!IS_ERR(key)) { x509 = last; diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c index cd455450b069..5e956c5b9071 100644 --- a/crypto/asymmetric_keys/pkcs7_verify.c +++ b/crypto/asymmetric_keys/pkcs7_verify.c @@ -187,11 +187,11 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7, goto maybe_missing_crypto_in_x509; pr_debug(- issuer %s\n, x509-issuer); - if (x509-authority) + if (x509-auth_skid) pr_debug(- authkeyid %*phN\n, - x509-authority-len, x509-authority-data); + x509-auth_skid-len, x509-auth_skid-data); - if (!x509-authority || + if (!x509-auth_skid || strcmp(x509-subject, x509-issuer) == 0) { /* If there's no authority certificate specified, then * the certificate must be self-signed and is the root @@ -216,13 +216,13 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7, * list to see if the next one is there. */ pr_debug(- want %*phN\n, - x509-authority-len, x509-authority-data); + x509-auth_skid-len, x509-auth_skid-data); for (p = pkcs7-certs; p; p = p-next) { if (!p-skid) continue; pr_debug(- cmp [%u] %*phN\n, p-index, p-skid-len, p-skid-data); - if (asymmetric_key_id_same(p-skid, x509-authority)) + if (asymmetric_key_id_same(p-skid, x509-auth_skid)) goto found_issuer; } @@ -338,8 +338,6 @@ int pkcs7_verify(struct pkcs7_message *pkcs7) ret = x509_get_sig_params(x509); if (ret 0) return ret; - pr_debug(X.509[%u] %*phN\n, - n, x509-authority-len, x509-authority-data); } for (sinfo = pkcs7-signed_infos; sinfo; sinfo = sinfo-next) { diff --git a/crypto/asymmetric_keys/x509_akid.asn1 b/crypto/asymmetric_keys/x509_akid.asn1 new file mode 100644 index ..1a33231a75a8 --- /dev/null +++
Re: [PATCH 1/5] X.509: Extract both parts of the AuthorityKeyIdentifier
On 21/11/14 16:42, Vivek Goyal wrote: On Thu, Nov 20, 2014 at 04:54:03PM +, David Howells wrote: [..] diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h index 3dfe6b5d6f0b..223b72344060 100644 --- a/crypto/asymmetric_keys/x509_parser.h +++ b/crypto/asymmetric_keys/x509_parser.h @@ -21,7 +21,8 @@ struct x509_certificate { char*subject; /* Name of certificate subject */ struct asymmetric_key_id *id; /* Serial number + issuer */ struct asymmetric_key_id *skid; /* Subject + subjectKeyId (optional) */ -struct asymmetric_key_id *authority;/* Authority key identifier (optional) */ +struct asymmetric_key_id *auth_id; /* CA AuthKeyId matching -id (optional) */ +struct asymmetric_key_id *auth_skid;/* CA AuthKeyId matching -skid (optional) */ A very minor nit. It might help if we put additional comment to explain what auth_id and auth_skid are composed of (like other key ids). auth_id /* akid issuer + akid serial */ auth_skid /* issuer + akid keyid */ Thanks Vivek Right, David did not address this in his v2 patchset... - Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] ima: Fix build failure on powerpc when TCG_IBMVTPM dependencies are not met
Hello, Yes, we will pick it up. Thanks, Dmitry On 03/12/14 08:04, Michael Ellerman wrote: > On powerpc we can end up with IMA=y and PPC_PSERIES=n which leads to: > > warning: (IMA) selects TCG_IBMVTPM which has unmet direct dependencies > (TCG_TPM && PPC_PSERIES) > tpm_ibmvtpm.c:(.text+0x14f3e8): undefined reference to `.plpar_hcall_norets' > > I'm not sure why IMA needs to select those user-visible symbols, but if > it must then the simplest fix is to just express the proper dependencies > on the select. > > Signed-off-by: Michael Ellerman > --- > security/integrity/ima/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > Could someone please pick this up? > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index e099875643c5..b51668d04f9d 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -10,7 +10,7 @@ config IMA > select CRYPTO_HASH_INFO > select TCG_TPM if HAS_IOMEM && !UML > select TCG_TIS if TCG_TPM && X86 > - select TCG_IBMVTPM if TCG_TPM && PPC64 > + select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES > help > The Trusted Computing Group(TCG) runtime Integrity > Measurement Architecture(IMA) maintains a list of hash -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RESEND PATCH] ima: Fix build failure on powerpc when TCG_IBMVTPM dependencies are not met
Hello, Yes, we will pick it up. Thanks, Dmitry On 03/12/14 08:04, Michael Ellerman wrote: On powerpc we can end up with IMA=y and PPC_PSERIES=n which leads to: warning: (IMA) selects TCG_IBMVTPM which has unmet direct dependencies (TCG_TPM PPC_PSERIES) tpm_ibmvtpm.c:(.text+0x14f3e8): undefined reference to `.plpar_hcall_norets' I'm not sure why IMA needs to select those user-visible symbols, but if it must then the simplest fix is to just express the proper dependencies on the select. Signed-off-by: Michael Ellerman m...@ellerman.id.au --- security/integrity/ima/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Could someone please pick this up? diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index e099875643c5..b51668d04f9d 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -10,7 +10,7 @@ config IMA select CRYPTO_HASH_INFO select TCG_TPM if HAS_IOMEM !UML select TCG_TIS if TCG_TPM X86 - select TCG_IBMVTPM if TCG_TPM PPC64 + select TCG_IBMVTPM if TCG_TPM PPC_PSERIES help The Trusted Computing Group(TCG) runtime Integrity Measurement Architecture(IMA) maintains a list of hash -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
On 20/11/14 18:53, David Howells wrote: > Here's a set of patches that does the following: > > (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. > We already extract the bit that can match the subjectKeyIdentifier (SKID) > of the parent X.509 cert, but we currently ignore the bits that can match > the issuer and serialNumber. > > Looks up an X.509 cert by issuer and serialNumber if those are provided > in > the AKID. If the keyIdentifier is also provided, checks that the > subjectKeyIdentifier of the cert found matches that also. > > If no issuer and serialNumber are provided in the AKID, looks up an X.509 > cert by SKID using the AKID keyIdentifier. > > This allows module signing to be done with certificates that don't have > an > SKID by which they can be looked up. > > (2) Makes use of the PKCS#7 facility to provide module signatures. > > sign-file is replaced with a program that generates a PKCS#7 message that > has no X.509 certs embedded and that has detached data (the module > content) and adds it onto the message with magic string and descriptor. Why do you highlight that X509 is not embedded? Current module signing does not embed X509 also. - Dmitry > (3) The PKCS#7 message (and matching X.509 cert) supply all the information > that is needed to select the X.509 cert to be used to verify the > signature > by standard means (including selection of digest algorithm and public key > algorithm). No kernel-specific magic values are required. > > Note that the revised sign-file program no longer supports the "-s > " > option as I'm not sure what the best way to deal with this is. Do we generate > a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I > lean towards the latter. > > They can be found here also: > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 > > These patches are based on the security tree's next branch. > > David > --- > David Howells (5): > X.509: Extract both parts of the AuthorityKeyIdentifier > X.509: Support X.509 lookup by Issuer+Serial form AuthorityKeyIdentifier > PKCS#7: Allow detached data to be supplied for signature checking > purposes > MODSIGN: Provide a utility to append a PKCS#7 signature to a module > MODSIGN: Use PKCS#7 messages as module signatures > > > crypto/asymmetric_keys/Makefile |8 - > crypto/asymmetric_keys/pkcs7_trust.c | 10 - > crypto/asymmetric_keys/pkcs7_verify.c | 81 -- > crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ > crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ > crypto/asymmetric_keys/x509_parser.h |3 > crypto/asymmetric_keys/x509_public_key.c | 85 -- > include/crypto/pkcs7.h|3 > include/crypto/public_key.h |4 > init/Kconfig |1 > kernel/module_signing.c | 220 +++ > scripts/Makefile |2 > scripts/sign-file | 421 > - > scripts/sign-file.c | 189 + > 14 files changed, 505 insertions(+), 699 deletions(-) > create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 > delete mode 100755 scripts/sign-file > create mode 100755 scripts/sign-file.c > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
On 21/11/14 14:59, Dmitry Kasatkin wrote: > Hi David, > > Before I go into reviewing the patches just want to let you know that > Integrity stuff seems to work fine with these changes. Actually after cleaning the tree and re-signing the modules, I get following Unrecognized character \x7F; marked by <-- HERE after <-- HERE near column 1 at ./scripts/sign-file line 1. make[1]: *** [arch/x86/crypto/aes-x86_64.ko] Error 255 - Dmitry > Thanks, > Dmitry > > On 20/11/14 18:53, David Howells wrote: >> Here's a set of patches that does the following: >> >> (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. >> We already extract the bit that can match the subjectKeyIdentifier >> (SKID) >> of the parent X.509 cert, but we currently ignore the bits that can >> match >> the issuer and serialNumber. >> >> Looks up an X.509 cert by issuer and serialNumber if those are provided >> in >> the AKID. If the keyIdentifier is also provided, checks that the >> subjectKeyIdentifier of the cert found matches that also. >> >> If no issuer and serialNumber are provided in the AKID, looks up an >> X.509 >> cert by SKID using the AKID keyIdentifier. >> >> This allows module signing to be done with certificates that don't have >> an >> SKID by which they can be looked up. >> >> (2) Makes use of the PKCS#7 facility to provide module signatures. >> >> sign-file is replaced with a program that generates a PKCS#7 message >> that >> has no X.509 certs embedded and that has detached data (the module >> content) and adds it onto the message with magic string and descriptor. >> >> (3) The PKCS#7 message (and matching X.509 cert) supply all the information >> that is needed to select the X.509 cert to be used to verify the >> signature >> by standard means (including selection of digest algorithm and public >> key >> algorithm). No kernel-specific magic values are required. >> >> Note that the revised sign-file program no longer supports the "-s >> " >> option as I'm not sure what the best way to deal with this is. Do we >> generate >> a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I >> lean towards the latter. >> >> They can be found here also: >> >> >> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 >> >> These patches are based on the security tree's next branch. >> >> David >> --- >> David Howells (5): >> X.509: Extract both parts of the AuthorityKeyIdentifier >> X.509: Support X.509 lookup by Issuer+Serial form >> AuthorityKeyIdentifier >> PKCS#7: Allow detached data to be supplied for signature checking >> purposes >> MODSIGN: Provide a utility to append a PKCS#7 signature to a module >> MODSIGN: Use PKCS#7 messages as module signatures >> >> >> crypto/asymmetric_keys/Makefile |8 - >> crypto/asymmetric_keys/pkcs7_trust.c | 10 - >> crypto/asymmetric_keys/pkcs7_verify.c | 81 -- >> crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ >> crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ >> crypto/asymmetric_keys/x509_parser.h |3 >> crypto/asymmetric_keys/x509_public_key.c | 85 -- >> include/crypto/pkcs7.h|3 >> include/crypto/public_key.h |4 >> init/Kconfig |1 >> kernel/module_signing.c | 220 +++ >> scripts/Makefile |2 >> scripts/sign-file | 421 >> - >> scripts/sign-file.c | 189 + >> 14 files changed, 505 insertions(+), 699 deletions(-) >> create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 >> delete mode 100755 scripts/sign-file >> create mode 100755 scripts/sign-file.c >> >> > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
On 21/11/14 14:59, Dmitry Kasatkin wrote: Hi David, Before I go into reviewing the patches just want to let you know that Integrity stuff seems to work fine with these changes. Actually after cleaning the tree and re-signing the modules, I get following Unrecognized character \x7F; marked by -- HERE after -- HERE near column 1 at ./scripts/sign-file line 1. make[1]: *** [arch/x86/crypto/aes-x86_64.ko] Error 255 - Dmitry Thanks, Dmitry On 20/11/14 18:53, David Howells wrote: Here's a set of patches that does the following: (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. We already extract the bit that can match the subjectKeyIdentifier (SKID) of the parent X.509 cert, but we currently ignore the bits that can match the issuer and serialNumber. Looks up an X.509 cert by issuer and serialNumber if those are provided in the AKID. If the keyIdentifier is also provided, checks that the subjectKeyIdentifier of the cert found matches that also. If no issuer and serialNumber are provided in the AKID, looks up an X.509 cert by SKID using the AKID keyIdentifier. This allows module signing to be done with certificates that don't have an SKID by which they can be looked up. (2) Makes use of the PKCS#7 facility to provide module signatures. sign-file is replaced with a program that generates a PKCS#7 message that has no X.509 certs embedded and that has detached data (the module content) and adds it onto the message with magic string and descriptor. (3) The PKCS#7 message (and matching X.509 cert) supply all the information that is needed to select the X.509 cert to be used to verify the signature by standard means (including selection of digest algorithm and public key algorithm). No kernel-specific magic values are required. Note that the revised sign-file program no longer supports the -s signature option as I'm not sure what the best way to deal with this is. Do we generate a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I lean towards the latter. They can be found here also: http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 These patches are based on the security tree's next branch. David --- David Howells (5): X.509: Extract both parts of the AuthorityKeyIdentifier X.509: Support X.509 lookup by Issuer+Serial form AuthorityKeyIdentifier PKCS#7: Allow detached data to be supplied for signature checking purposes MODSIGN: Provide a utility to append a PKCS#7 signature to a module MODSIGN: Use PKCS#7 messages as module signatures crypto/asymmetric_keys/Makefile |8 - crypto/asymmetric_keys/pkcs7_trust.c | 10 - crypto/asymmetric_keys/pkcs7_verify.c | 81 -- crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ crypto/asymmetric_keys/x509_parser.h |3 crypto/asymmetric_keys/x509_public_key.c | 85 -- include/crypto/pkcs7.h|3 include/crypto/public_key.h |4 init/Kconfig |1 kernel/module_signing.c | 220 +++ scripts/Makefile |2 scripts/sign-file | 421 - scripts/sign-file.c | 189 + 14 files changed, 505 insertions(+), 699 deletions(-) create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 delete mode 100755 scripts/sign-file create mode 100755 scripts/sign-file.c -- To unsubscribe from this list: send the line unsubscribe linux-security-module in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
On 20/11/14 18:53, David Howells wrote: Here's a set of patches that does the following: (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. We already extract the bit that can match the subjectKeyIdentifier (SKID) of the parent X.509 cert, but we currently ignore the bits that can match the issuer and serialNumber. Looks up an X.509 cert by issuer and serialNumber if those are provided in the AKID. If the keyIdentifier is also provided, checks that the subjectKeyIdentifier of the cert found matches that also. If no issuer and serialNumber are provided in the AKID, looks up an X.509 cert by SKID using the AKID keyIdentifier. This allows module signing to be done with certificates that don't have an SKID by which they can be looked up. (2) Makes use of the PKCS#7 facility to provide module signatures. sign-file is replaced with a program that generates a PKCS#7 message that has no X.509 certs embedded and that has detached data (the module content) and adds it onto the message with magic string and descriptor. Why do you highlight that X509 is not embedded? Current module signing does not embed X509 also. - Dmitry (3) The PKCS#7 message (and matching X.509 cert) supply all the information that is needed to select the X.509 cert to be used to verify the signature by standard means (including selection of digest algorithm and public key algorithm). No kernel-specific magic values are required. Note that the revised sign-file program no longer supports the -s signature option as I'm not sure what the best way to deal with this is. Do we generate a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I lean towards the latter. They can be found here also: http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 These patches are based on the security tree's next branch. David --- David Howells (5): X.509: Extract both parts of the AuthorityKeyIdentifier X.509: Support X.509 lookup by Issuer+Serial form AuthorityKeyIdentifier PKCS#7: Allow detached data to be supplied for signature checking purposes MODSIGN: Provide a utility to append a PKCS#7 signature to a module MODSIGN: Use PKCS#7 messages as module signatures crypto/asymmetric_keys/Makefile |8 - crypto/asymmetric_keys/pkcs7_trust.c | 10 - crypto/asymmetric_keys/pkcs7_verify.c | 81 -- crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ crypto/asymmetric_keys/x509_parser.h |3 crypto/asymmetric_keys/x509_public_key.c | 85 -- include/crypto/pkcs7.h|3 include/crypto/public_key.h |4 init/Kconfig |1 kernel/module_signing.c | 220 +++ scripts/Makefile |2 scripts/sign-file | 421 - scripts/sign-file.c | 189 + 14 files changed, 505 insertions(+), 699 deletions(-) create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 delete mode 100755 scripts/sign-file create mode 100755 scripts/sign-file.c -- To unsubscribe from this list: send the line unsubscribe linux-security-module in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
Hi David, Before I go into reviewing the patches just want to let you know that Integrity stuff seems to work fine with these changes. Thanks, Dmitry On 20/11/14 18:53, David Howells wrote: > Here's a set of patches that does the following: > > (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. > We already extract the bit that can match the subjectKeyIdentifier (SKID) > of the parent X.509 cert, but we currently ignore the bits that can match > the issuer and serialNumber. > > Looks up an X.509 cert by issuer and serialNumber if those are provided > in > the AKID. If the keyIdentifier is also provided, checks that the > subjectKeyIdentifier of the cert found matches that also. > > If no issuer and serialNumber are provided in the AKID, looks up an X.509 > cert by SKID using the AKID keyIdentifier. > > This allows module signing to be done with certificates that don't have > an > SKID by which they can be looked up. > > (2) Makes use of the PKCS#7 facility to provide module signatures. > > sign-file is replaced with a program that generates a PKCS#7 message that > has no X.509 certs embedded and that has detached data (the module > content) and adds it onto the message with magic string and descriptor. > > (3) The PKCS#7 message (and matching X.509 cert) supply all the information > that is needed to select the X.509 cert to be used to verify the > signature > by standard means (including selection of digest algorithm and public key > algorithm). No kernel-specific magic values are required. > > Note that the revised sign-file program no longer supports the "-s > " > option as I'm not sure what the best way to deal with this is. Do we generate > a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I > lean towards the latter. > > They can be found here also: > > > http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 > > These patches are based on the security tree's next branch. > > David > --- > David Howells (5): > X.509: Extract both parts of the AuthorityKeyIdentifier > X.509: Support X.509 lookup by Issuer+Serial form AuthorityKeyIdentifier > PKCS#7: Allow detached data to be supplied for signature checking > purposes > MODSIGN: Provide a utility to append a PKCS#7 signature to a module > MODSIGN: Use PKCS#7 messages as module signatures > > > crypto/asymmetric_keys/Makefile |8 - > crypto/asymmetric_keys/pkcs7_trust.c | 10 - > crypto/asymmetric_keys/pkcs7_verify.c | 81 -- > crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ > crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ > crypto/asymmetric_keys/x509_parser.h |3 > crypto/asymmetric_keys/x509_public_key.c | 85 -- > include/crypto/pkcs7.h|3 > include/crypto/public_key.h |4 > init/Kconfig |1 > kernel/module_signing.c | 220 +++ > scripts/Makefile |2 > scripts/sign-file | 421 > - > scripts/sign-file.c | 189 + > 14 files changed, 505 insertions(+), 699 deletions(-) > create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 > delete mode 100755 scripts/sign-file > create mode 100755 scripts/sign-file.c > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] MODSIGN: Use PKCS#7 for module signatures
Hi David, Before I go into reviewing the patches just want to let you know that Integrity stuff seems to work fine with these changes. Thanks, Dmitry On 20/11/14 18:53, David Howells wrote: Here's a set of patches that does the following: (1) Extracts both parts of an X.509 AuthorityKeyIdentifier (AKID) extension. We already extract the bit that can match the subjectKeyIdentifier (SKID) of the parent X.509 cert, but we currently ignore the bits that can match the issuer and serialNumber. Looks up an X.509 cert by issuer and serialNumber if those are provided in the AKID. If the keyIdentifier is also provided, checks that the subjectKeyIdentifier of the cert found matches that also. If no issuer and serialNumber are provided in the AKID, looks up an X.509 cert by SKID using the AKID keyIdentifier. This allows module signing to be done with certificates that don't have an SKID by which they can be looked up. (2) Makes use of the PKCS#7 facility to provide module signatures. sign-file is replaced with a program that generates a PKCS#7 message that has no X.509 certs embedded and that has detached data (the module content) and adds it onto the message with magic string and descriptor. (3) The PKCS#7 message (and matching X.509 cert) supply all the information that is needed to select the X.509 cert to be used to verify the signature by standard means (including selection of digest algorithm and public key algorithm). No kernel-specific magic values are required. Note that the revised sign-file program no longer supports the -s signature option as I'm not sure what the best way to deal with this is. Do we generate a PKCS#7 cert from the signature given, or do we get given a PKCS#7 cert? I lean towards the latter. They can be found here also: http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7 These patches are based on the security tree's next branch. David --- David Howells (5): X.509: Extract both parts of the AuthorityKeyIdentifier X.509: Support X.509 lookup by Issuer+Serial form AuthorityKeyIdentifier PKCS#7: Allow detached data to be supplied for signature checking purposes MODSIGN: Provide a utility to append a PKCS#7 signature to a module MODSIGN: Use PKCS#7 messages as module signatures crypto/asymmetric_keys/Makefile |8 - crypto/asymmetric_keys/pkcs7_trust.c | 10 - crypto/asymmetric_keys/pkcs7_verify.c | 81 -- crypto/asymmetric_keys/x509_akid.asn1 | 35 ++ crypto/asymmetric_keys/x509_cert_parser.c | 142 ++ crypto/asymmetric_keys/x509_parser.h |3 crypto/asymmetric_keys/x509_public_key.c | 85 -- include/crypto/pkcs7.h|3 include/crypto/public_key.h |4 init/Kconfig |1 kernel/module_signing.c | 220 +++ scripts/Makefile |2 scripts/sign-file | 421 - scripts/sign-file.c | 189 + 14 files changed, 505 insertions(+), 699 deletions(-) create mode 100644 crypto/asymmetric_keys/x509_akid.asn1 delete mode 100755 scripts/sign-file create mode 100755 scripts/sign-file.c -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 2/6] integrity: provide a function to load x509 certificate from the kernel
Provide the function to load x509 certificates from the kernel into the integrity kernel keyring. Changes in v2: * configuration option removed * function declared as '__init' Signed-off-by: Dmitry Kasatkin --- security/integrity/digsig.c| 37 - security/integrity/integrity.h | 2 ++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 4f643d1..fa383fd 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -14,7 +14,7 @@ #include #include -#include +#include #include #include #include @@ -84,3 +84,38 @@ int __init integrity_init_keyring(const unsigned int id) } return err; } + +int __init integrity_load_x509(const unsigned int id, char *path) +{ + key_ref_t key; + char *data; + int rc; + + if (!keyring[id]) + return -EINVAL; + + rc = integrity_read_file(path, ); + if (rc < 0) + return rc; + + key = key_create_or_update(make_key_ref(keyring[id], 1), + "asymmetric", + NULL, + data, + rc, + ((KEY_POS_ALL & ~KEY_POS_SETATTR) | + KEY_USR_VIEW | KEY_USR_READ), + KEY_ALLOC_NOT_IN_QUOTA | + KEY_ALLOC_TRUSTED); + if (IS_ERR(key)) { + rc = PTR_ERR(key); + pr_err("Problem loading X.509 certificate (%d): %s\n", + rc, path); + } else { + pr_notice("Loaded X.509 cert '%s': %s\n", + key_ref_to_ptr(key)->description, path); + key_ref_put(key); + } + kfree(data); + return 0; +} diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 20d2204..1057abb 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -134,6 +134,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, const char *digest, int digestlen); int __init integrity_init_keyring(const unsigned int id); +int __init integrity_load_x509(const unsigned int id, char *path); #else static inline int integrity_digsig_verify(const unsigned int id, @@ -147,6 +148,7 @@ static inline int integrity_init_keyring(const unsigned int id) { return 0; } + #endif /* CONFIG_INTEGRITY_SIGNATURE */ #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 1/6] integrity: define a new function integrity_read_file()
This patch defines a new function called integrity_read_file() to read file from the kernel into a buffer. Subsequent patches will read a file containing the public keys and load them onto the IMA keyring. This patch moves and renames ima_kernel_read(), the non-security checking version of kernel_read(), to integrity_kernel_read(). Changes in v3: * Patch descriptions improved (Mimi) Changes in v2: * configuration option removed * function declared as '__init' Signed-off-by: Dmitry Kasatkin --- security/integrity/iint.c | 78 + security/integrity/ima/ima_crypto.c | 35 ++--- security/integrity/integrity.h | 4 ++ 3 files changed, 85 insertions(+), 32 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index cc3eb4d..0a76686 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include "integrity.h" static struct rb_root integrity_iint_tree = RB_ROOT; @@ -167,3 +169,79 @@ static int __init integrity_iintcache_init(void) return 0; } security_initcall(integrity_iintcache_init); + + +/* + * integrity_kernel_read - read data from the file + * + * This is a function for reading file content instead of kernel_read(). + * It does not perform locking checks to ensure it cannot be blocked. + * It does not perform security checks because it is irrelevant for IMA. + * + */ +int integrity_kernel_read(struct file *file, loff_t offset, + char *addr, unsigned long count) +{ + mm_segment_t old_fs; + char __user *buf = addr; + ssize_t ret = -EINVAL; + + if (!(file->f_mode & FMODE_READ)) + return -EBADF; + + old_fs = get_fs(); + set_fs(get_ds()); + if (file->f_op->read) + ret = file->f_op->read(file, buf, count, ); + else if (file->f_op->aio_read) + ret = do_sync_read(file, buf, count, ); + else if (file->f_op->read_iter) + ret = new_sync_read(file, buf, count, ); + set_fs(old_fs); + return ret; +} + +/* + * integrity_read_file - read entire file content into the buffer + * + * This is function opens a file, allocates the buffer of required + * size, read entire file content to the buffer and closes the file + * + * It is used only by init code. + * + */ +int __init integrity_read_file(const char *path, char **data) +{ + struct file *file; + loff_t size; + char *buf; + int rc = -EINVAL; + + file = filp_open(path, O_RDONLY, 0); + if (IS_ERR(file)) { + rc = PTR_ERR(file); + pr_err("Unable to open file: %s (%d)", path, rc); + return rc; + } + + size = i_size_read(file_inode(file)); + if (size <= 0) + goto out; + + buf = kmalloc(size, GFP_KERNEL); + if (!buf) { + rc = -ENOMEM; + goto out; + } + + rc = integrity_kernel_read(file, 0, buf, size); + if (rc < 0) + kfree(buf); + else if (rc != size) + rc = -EIO; + else + *data = buf; +out: + fput(file); + return rc; +} diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c index d34e7df..5df4d96 100644 --- a/security/integrity/ima/ima_crypto.c +++ b/security/integrity/ima/ima_crypto.c @@ -67,36 +67,6 @@ MODULE_PARM_DESC(ahash_bufsize, "Maximum ahash buffer size"); static struct crypto_shash *ima_shash_tfm; static struct crypto_ahash *ima_ahash_tfm; -/** - * ima_kernel_read - read file content - * - * This is a function for reading file content instead of kernel_read(). - * It does not perform locking checks to ensure it cannot be blocked. - * It does not perform security checks because it is irrelevant for IMA. - * - */ -static int ima_kernel_read(struct file *file, loff_t offset, - char *addr, unsigned long count) -{ - mm_segment_t old_fs; - char __user *buf = addr; - ssize_t ret = -EINVAL; - - if (!(file->f_mode & FMODE_READ)) - return -EBADF; - - old_fs = get_fs(); - set_fs(get_ds()); - if (file->f_op->read) - ret = file->f_op->read(file, buf, count, ); - else if (file->f_op->aio_read) - ret = do_sync_read(file, buf, count, ); - else if (file->f_op->read_iter) - ret = new_sync_read(file, buf, count, ); - set_fs(old_fs); - return ret; -} - int __init ima_init_crypto(void) { long rc; @@ -324,7 +294,8 @@ static int ima_calc_file_hash_atfm(struct file *file, } /* read buffer */ rbuf_len = min_t(loff_t, i_size - offset, rbuf_size[active]); - rc = ima_kernel_read(fi
[PATCH v4 5/6] ima: require signature based appraisal
This patch provides CONFIG_IMA_APPRAISE_SIGNED_INIT kernel configuration option to force IMA appraisal using signatures. This is useful, when EVM key is not initialized yet and we want securely initialize integrity or any other functionality. It forces embedded policy to require signature. Signed initialization script can initialize EVM key, update the IMA policy and change further requirement of everything to be signed. Changes in v3: * kernel parameter fixed to configuration option in the patch description Changes in v2: * policy change of this patch separated from the key loading patch Signed-off-by: Dmitry Kasatkin --- security/integrity/ima/Kconfig | 7 +++ security/integrity/ima/ima_policy.c | 5 + 2 files changed, 12 insertions(+) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 8288edc..31b44b8 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -146,3 +146,10 @@ config IMA_X509_PATH default "/etc/keys/x509_ima.der" help This option defines IMA X509 certificate path. + +config IMA_APPRAISE_SIGNED_INIT + bool "Require signed user-space initialization" + depends on IMA_LOAD_X509 + default n + help + This option requires user-space init to be signed. diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 0d14d25..222ff79 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -100,7 +100,12 @@ static struct ima_rule_entry default_appraise_rules[] = { {.action = DONT_APPRAISE, .fsmagic = SECURITYFS_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_APPRAISE, .fsmagic = SELINUX_MAGIC, .flags = IMA_FSMAGIC}, {.action = DONT_APPRAISE, .fsmagic = CGROUP_SUPER_MAGIC, .flags = IMA_FSMAGIC}, +#ifndef CONFIG_IMA_APPRAISE_SIGNED_INIT {.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .flags = IMA_FOWNER}, +#else + /* force signature */ + {.action = APPRAISE, .fowner = GLOBAL_ROOT_UID, .flags = IMA_FOWNER | IMA_DIGSIG_REQUIRED}, +#endif }; static LIST_HEAD(ima_default_rules); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 0/6] ima: provide signature based 'init' appraisal
Currently secure IMA/EVM initialization has to be done from the initramfs, embedded in the signed kernel image. Many systems do not want to use initramfs or usage of embedded initramfs makes it difficult to have multi-target kernels. This is a very simple patchset which makes it possible to perform secure initialization by requiring initial user-space to be signed. It does it by: - introducing a hook to load keys - loading IMA signed public key certificate into the '.ima' trusted keyring - making default IMA appraisal policy to require everything to be signed When builtin initramfs is not in use, keys cannot be read from initcalls, because root filesystem is not yet mounted. In order to read keys before executing init process, ima_prepare_keys() hook is introduced. Reading public keys from the kernel is justified because signature verification key is needed in order to verify anything else which is read from the file system. Public keys are X509 certificates and itself signed by the trusted key from the .system keyring. Kernel BIG KEYS support is an example of reading keys directly by the kernel. CONFIG_IMA_APPRAISE_SIGNED_INIT kernel option is provided to make the IMA default appraisal policy to required signature validation. Signed init process need to initialize EVM key and load appropriate IMA policy which would not require everything to be signed. Unless real '/sbin/init' is signed, a simple and practical way is to place all signed programs, libraries, scripts and configuration files under dedicated directory, for example '/ima', and run signed init process by providing a kernel command line parameter 'init=/ima/init'. In the first post of these patches Andrew Morton noted that integrity_read_file() is a very simple open-file-and-slurp-it-into-memory and if there are other similar functions that can be made in ./lib. I found out that only sound:sound_firmware.c:do_mod_firmware_load(), which is enabled by CONFIG_SOUND_PRIME which is related to deprecated OSS interface and is not enabled anymore in latest Ubuntu kernels, at least. So I am keeping integrity_read_file() in integrity subsystem. cpio based initramfs currently does not support extended attributes. There is an initial agreement to introduce light-weight tar parser to the kernel to support extended attributes which will make it possible to use IMA appraisal with external initramfs. It will benefit from this patchset and allow to update initramfs with signed files also on the running system as distros do. Changes in v4: * use ima_policy_flag to disable appraisal * key directory path changed to /etc/keys (Mimi) * slightly updated patch descriptions Changes in v3: * ima_prepare_keys() renamed to integrity_load_keys() to be the hook for both modules of integrity subsystem IMA/EVM. * removed unnecessary configuration options and declared init functions with '__init'. * updated to lately introduced 'ima_policy_flag' variable to disabled and enable IMA appraisal. * separated key loading patch from policy change patch * added patch which refactor vfs_read(). Agreed with Mimi to offer to move calling file operations hooks to a separate helper function which is then used by vfs_read() and integrity_kernel_read(). Applying this patch does not affect functionality and can be applied if agreed so. Changes in v2: * ima_kernel_read() moved as integrity_kernel_read() from ima_crypto.c to iint.c for use by integrity_read_file. The reason for keeping internal version is because 'integrity' version does not call fsnotify_access(), add_rchar() and inc_syscr(). * integrity_read_file() moved from digsig.c to iint.c because it is used by IMA crypto subsystem and should not depend on digsig support being enabled. -Dmitry *** BLURB HERE *** Dmitry Kasatkin (6): integrity: define a new function integrity_read_file() integrity: provide a function to load x509 certificate from the kernel ima: load x509 certificate from the kernel integrity: provide a hook to load keys when rootfs is ready ima: require signature based appraisal VFS: refactor vfs_read() fs/read_write.c | 24 --- include/linux/fs.h | 1 + include/linux/integrity.h | 6 +++ init/main.c | 6 ++- security/integrity/digsig.c | 37 +++- security/integrity/iint.c | 85 + security/integrity/ima/Kconfig | 22 ++ security/integrity/ima/ima_api.c| 3 +- security/integrity/ima/ima_crypto.c | 35 ++- security/integrity/ima/ima_init.c | 17 security/integrity/ima/ima_policy.c | 5 +++ security/integrity/integrity.h | 14 ++ 12 files changed, 213 insertions(+), 42 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org
[PATCH v4 6/6] VFS: refactor vfs_read()
integrity_kernel_read() duplicates the file read operations code in vfs_read(). This patch refactors vfs_read() code creating a helper function __vfs_read(). It is used by both vfs_read() and integrity_kernel_read(). Signed-off-by: Dmitry Kasatkin --- fs/read_write.c | 24 ++-- include/linux/fs.h| 1 + security/integrity/iint.c | 10 +++--- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/fs/read_write.c b/fs/read_write.c index 009d854..f45b2ae 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -412,6 +412,23 @@ ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, loff_t *p EXPORT_SYMBOL(new_sync_read); +ssize_t __vfs_read(struct file *file, char __user *buf, size_t count, + loff_t *pos) +{ + ssize_t ret; + + if (file->f_op->read) + ret = file->f_op->read(file, buf, count, pos); + else if (file->f_op->aio_read) + ret = do_sync_read(file, buf, count, pos); + else if (file->f_op->read_iter) + ret = new_sync_read(file, buf, count, pos); + else + ret = -EINVAL; + + return ret; +} + ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos) { ssize_t ret; @@ -426,12 +443,7 @@ ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos) ret = rw_verify_area(READ, file, pos, count); if (ret >= 0) { count = ret; - if (file->f_op->read) - ret = file->f_op->read(file, buf, count, pos); - else if (file->f_op->aio_read) - ret = do_sync_read(file, buf, count, pos); - else - ret = new_sync_read(file, buf, count, pos); + ret = __vfs_read(file, buf, count, pos); if (ret > 0) { fsnotify_access(file); add_rchar(current, ret); diff --git a/include/linux/fs.h b/include/linux/fs.h index e11d60c..ac3a36e 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1527,6 +1527,7 @@ ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector, struct iovec *fast_pointer, struct iovec **ret_pointer); +extern ssize_t __vfs_read(struct file *, char __user *, size_t, loff_t *); extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *); extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *); extern ssize_t vfs_readv(struct file *, const struct iovec __user *, diff --git a/security/integrity/iint.c b/security/integrity/iint.c index a1f5cd1..e359477 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -184,20 +184,16 @@ int integrity_kernel_read(struct file *file, loff_t offset, { mm_segment_t old_fs; char __user *buf = addr; - ssize_t ret = -EINVAL; + ssize_t ret; if (!(file->f_mode & FMODE_READ)) return -EBADF; old_fs = get_fs(); set_fs(get_ds()); - if (file->f_op->read) - ret = file->f_op->read(file, buf, count, ); - else if (file->f_op->aio_read) - ret = do_sync_read(file, buf, count, ); - else if (file->f_op->read_iter) - ret = new_sync_read(file, buf, count, ); + ret = __vfs_read(file, buf, count, ); set_fs(old_fs); + return ret; } -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 3/6] ima: load x509 certificate from the kernel
Define configuration option to load X509 certificate into the IMA trusted kernel keyring. It implements ima_load_x509() hook to load X509 certificate into the .ima trusted kernel keyring from the root filesystem. Changes in v3: * use ima_policy_flag in ima_get_action() ima_load_x509 temporarily clears ima_policy_flag to disable appraisal to load key. Use it to skip appraisal rules. * Key directory path changed to /etc/keys (Mimi) Changes in v2: * added '__init' * use ima_policy_flag to disable appraisal to load keys Signed-off-by: Dmitry Kasatkin --- security/integrity/ima/Kconfig| 15 +++ security/integrity/ima/ima_api.c | 3 +-- security/integrity/ima/ima_init.c | 17 + security/integrity/integrity.h| 8 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index e099875..8288edc 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -131,3 +131,18 @@ config IMA_TRUSTED_KEYRING help This option requires that all keys added to the .ima keyring be signed by a key on the system trusted keyring. + +config IMA_LOAD_X509 + bool "Load X509 certificate to the '.ima' trusted keyring" + depends on IMA_TRUSTED_KEYRING + default n + help + This option enables X509 certificate loading from the kernel + to the '.ima' trusted keyring. + +config IMA_X509_PATH + string "IMA X509 certificate path" + depends on IMA_LOAD_X509 + default "/etc/keys/x509_ima.der" + help + This option defines IMA X509 certificate path. diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index a99eb6d..b0dc922 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -173,8 +173,7 @@ int ima_get_action(struct inode *inode, int mask, int function) { int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE; - if (!ima_appraise) - flags &= ~IMA_APPRAISE; + flags &= ima_policy_flag; return ima_match_policy(inode, function, mask, flags); } diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 9164fc8..5e4c29d 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -24,6 +24,12 @@ #include #include "ima.h" +#ifdef CONFIG_IMA_X509_PATH +#define IMA_X509_PATH CONFIG_IMA_X509_PATH +#else +#define IMA_X509_PATH "/etc/keys/x509_ima.der" +#endif + /* name for boot aggregate entry */ static const char *boot_aggregate_name = "boot_aggregate"; int ima_used_chip; @@ -91,6 +97,17 @@ err_out: return result; } +#ifdef CONFIG_IMA_LOAD_X509 +void __init ima_load_x509(void) +{ + int unset_flags = ima_policy_flag & IMA_APPRAISE; + + ima_policy_flag &= ~unset_flags; + integrity_load_x509(INTEGRITY_KEYRING_IMA, IMA_X509_PATH); + ima_policy_flag |= unset_flags; +} +#endif + int __init ima_init(void) { u8 pcr_i[TPM_DIGEST_SIZE]; diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 1057abb..caa1f6c 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -162,6 +162,14 @@ static inline int asymmetric_verify(struct key *keyring, const char *sig, } #endif +#ifdef CONFIG_IMA_LOAD_X509 +void __init ima_load_x509(void); +#else +static inline void ima_load_x509(void) +{ +} +#endif + #ifdef CONFIG_INTEGRITY_AUDIT /* declarations */ void integrity_audit_msg(int audit_msgno, struct inode *inode, -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 4/6] integrity: provide a hook to load keys when rootfs is ready
Keys can only be loaded once the rootfs is mounted. Initcalls are not suitable for that. This patch defines a special hook to load the x509 public keys onto the IMA keyring, before attempting to access any file. The keys are required for verifying the file's signature. The hook is called after the root filesystem is mounted and before the kernel calls 'init'. Changes in v3: * added more explanation to the patch description (Mimi) Changes in v2: * Hook renamed as 'integrity_load_keys()' to handle both IMA and EVM keys by integrity subsystem. * Hook patch moved after defining loading functions Signed-off-by: Dmitry Kasatkin --- include/linux/integrity.h | 6 ++ init/main.c | 6 +- security/integrity/iint.c | 11 +++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/include/linux/integrity.h b/include/linux/integrity.h index 83222ce..c2d6082 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -24,6 +24,7 @@ enum integrity_status { #ifdef CONFIG_INTEGRITY extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode); extern void integrity_inode_free(struct inode *inode); +extern void __init integrity_load_keys(void); #else static inline struct integrity_iint_cache * @@ -36,5 +37,10 @@ static inline void integrity_inode_free(struct inode *inode) { return; } + +static inline void integrity_load_keys(void) +{ +} #endif /* CONFIG_INTEGRITY */ + #endif /* _LINUX_INTEGRITY_H */ diff --git a/init/main.c b/init/main.c index e8ae1fe..2c1928d 100644 --- a/init/main.c +++ b/init/main.c @@ -78,6 +78,7 @@ #include #include #include +#include #include #include @@ -1026,8 +1027,11 @@ static noinline void __init kernel_init_freeable(void) * Ok, we have completed the initial bootup, and * we're essentially up and running. Get rid of the * initmem segments and start the user-mode stuff.. +* +* rootfs is available now, try loading the public keys +* and default modules */ - /* rootfs is available now, try loading default modules */ + integrity_load_keys(); load_default_modules(); } diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 0a76686..a1f5cd1 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -245,3 +245,14 @@ out: fput(file); return rc; } + +/* + * integrity_load_keys - load integrity keys hook + * + * Hooks is called from init/main.c:kernel_init_freeable() + * when rootfs is ready + */ +void __init integrity_load_keys(void) +{ + ima_load_x509(); +} -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 4/6] integrity: provide a hook to load keys when rootfs is ready
Keys can only be loaded once the rootfs is mounted. Initcalls are not suitable for that. This patch defines a special hook to load the x509 public keys onto the IMA keyring, before attempting to access any file. The keys are required for verifying the file's signature. The hook is called after the root filesystem is mounted and before the kernel calls 'init'. Changes in v3: * added more explanation to the patch description (Mimi) Changes in v2: * Hook renamed as 'integrity_load_keys()' to handle both IMA and EVM keys by integrity subsystem. * Hook patch moved after defining loading functions Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- include/linux/integrity.h | 6 ++ init/main.c | 6 +- security/integrity/iint.c | 11 +++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/include/linux/integrity.h b/include/linux/integrity.h index 83222ce..c2d6082 100644 --- a/include/linux/integrity.h +++ b/include/linux/integrity.h @@ -24,6 +24,7 @@ enum integrity_status { #ifdef CONFIG_INTEGRITY extern struct integrity_iint_cache *integrity_inode_get(struct inode *inode); extern void integrity_inode_free(struct inode *inode); +extern void __init integrity_load_keys(void); #else static inline struct integrity_iint_cache * @@ -36,5 +37,10 @@ static inline void integrity_inode_free(struct inode *inode) { return; } + +static inline void integrity_load_keys(void) +{ +} #endif /* CONFIG_INTEGRITY */ + #endif /* _LINUX_INTEGRITY_H */ diff --git a/init/main.c b/init/main.c index e8ae1fe..2c1928d 100644 --- a/init/main.c +++ b/init/main.c @@ -78,6 +78,7 @@ #include linux/context_tracking.h #include linux/random.h #include linux/list.h +#include linux/integrity.h #include asm/io.h #include asm/bugs.h @@ -1026,8 +1027,11 @@ static noinline void __init kernel_init_freeable(void) * Ok, we have completed the initial bootup, and * we're essentially up and running. Get rid of the * initmem segments and start the user-mode stuff.. +* +* rootfs is available now, try loading the public keys +* and default modules */ - /* rootfs is available now, try loading default modules */ + integrity_load_keys(); load_default_modules(); } diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 0a76686..a1f5cd1 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -245,3 +245,14 @@ out: fput(file); return rc; } + +/* + * integrity_load_keys - load integrity keys hook + * + * Hooks is called from init/main.c:kernel_init_freeable() + * when rootfs is ready + */ +void __init integrity_load_keys(void) +{ + ima_load_x509(); +} -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 0/6] ima: provide signature based 'init' appraisal
Currently secure IMA/EVM initialization has to be done from the initramfs, embedded in the signed kernel image. Many systems do not want to use initramfs or usage of embedded initramfs makes it difficult to have multi-target kernels. This is a very simple patchset which makes it possible to perform secure initialization by requiring initial user-space to be signed. It does it by: - introducing a hook to load keys - loading IMA signed public key certificate into the '.ima' trusted keyring - making default IMA appraisal policy to require everything to be signed When builtin initramfs is not in use, keys cannot be read from initcalls, because root filesystem is not yet mounted. In order to read keys before executing init process, ima_prepare_keys() hook is introduced. Reading public keys from the kernel is justified because signature verification key is needed in order to verify anything else which is read from the file system. Public keys are X509 certificates and itself signed by the trusted key from the .system keyring. Kernel BIG KEYS support is an example of reading keys directly by the kernel. CONFIG_IMA_APPRAISE_SIGNED_INIT kernel option is provided to make the IMA default appraisal policy to required signature validation. Signed init process need to initialize EVM key and load appropriate IMA policy which would not require everything to be signed. Unless real '/sbin/init' is signed, a simple and practical way is to place all signed programs, libraries, scripts and configuration files under dedicated directory, for example '/ima', and run signed init process by providing a kernel command line parameter 'init=/ima/init'. In the first post of these patches Andrew Morton noted that integrity_read_file() is a very simple open-file-and-slurp-it-into-memory and if there are other similar functions that can be made in ./lib. I found out that only sound:sound_firmware.c:do_mod_firmware_load(), which is enabled by CONFIG_SOUND_PRIME which is related to deprecated OSS interface and is not enabled anymore in latest Ubuntu kernels, at least. So I am keeping integrity_read_file() in integrity subsystem. cpio based initramfs currently does not support extended attributes. There is an initial agreement to introduce light-weight tar parser to the kernel to support extended attributes which will make it possible to use IMA appraisal with external initramfs. It will benefit from this patchset and allow to update initramfs with signed files also on the running system as distros do. Changes in v4: * use ima_policy_flag to disable appraisal * key directory path changed to /etc/keys (Mimi) * slightly updated patch descriptions Changes in v3: * ima_prepare_keys() renamed to integrity_load_keys() to be the hook for both modules of integrity subsystem IMA/EVM. * removed unnecessary configuration options and declared init functions with '__init'. * updated to lately introduced 'ima_policy_flag' variable to disabled and enable IMA appraisal. * separated key loading patch from policy change patch * added patch which refactor vfs_read(). Agreed with Mimi to offer to move calling file operations hooks to a separate helper function which is then used by vfs_read() and integrity_kernel_read(). Applying this patch does not affect functionality and can be applied if agreed so. Changes in v2: * ima_kernel_read() moved as integrity_kernel_read() from ima_crypto.c to iint.c for use by integrity_read_file. The reason for keeping internal version is because 'integrity' version does not call fsnotify_access(), add_rchar() and inc_syscr(). * integrity_read_file() moved from digsig.c to iint.c because it is used by IMA crypto subsystem and should not depend on digsig support being enabled. -Dmitry *** BLURB HERE *** Dmitry Kasatkin (6): integrity: define a new function integrity_read_file() integrity: provide a function to load x509 certificate from the kernel ima: load x509 certificate from the kernel integrity: provide a hook to load keys when rootfs is ready ima: require signature based appraisal VFS: refactor vfs_read() fs/read_write.c | 24 --- include/linux/fs.h | 1 + include/linux/integrity.h | 6 +++ init/main.c | 6 ++- security/integrity/digsig.c | 37 +++- security/integrity/iint.c | 85 + security/integrity/ima/Kconfig | 22 ++ security/integrity/ima/ima_api.c| 3 +- security/integrity/ima/ima_crypto.c | 35 ++- security/integrity/ima/ima_init.c | 17 security/integrity/ima/ima_policy.c | 5 +++ security/integrity/integrity.h | 14 ++ 12 files changed, 213 insertions(+), 42 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo
[PATCH v4 3/6] ima: load x509 certificate from the kernel
Define configuration option to load X509 certificate into the IMA trusted kernel keyring. It implements ima_load_x509() hook to load X509 certificate into the .ima trusted kernel keyring from the root filesystem. Changes in v3: * use ima_policy_flag in ima_get_action() ima_load_x509 temporarily clears ima_policy_flag to disable appraisal to load key. Use it to skip appraisal rules. * Key directory path changed to /etc/keys (Mimi) Changes in v2: * added '__init' * use ima_policy_flag to disable appraisal to load keys Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- security/integrity/ima/Kconfig| 15 +++ security/integrity/ima/ima_api.c | 3 +-- security/integrity/ima/ima_init.c | 17 + security/integrity/integrity.h| 8 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index e099875..8288edc 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -131,3 +131,18 @@ config IMA_TRUSTED_KEYRING help This option requires that all keys added to the .ima keyring be signed by a key on the system trusted keyring. + +config IMA_LOAD_X509 + bool Load X509 certificate to the '.ima' trusted keyring + depends on IMA_TRUSTED_KEYRING + default n + help + This option enables X509 certificate loading from the kernel + to the '.ima' trusted keyring. + +config IMA_X509_PATH + string IMA X509 certificate path + depends on IMA_LOAD_X509 + default /etc/keys/x509_ima.der + help + This option defines IMA X509 certificate path. diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index a99eb6d..b0dc922 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -173,8 +173,7 @@ int ima_get_action(struct inode *inode, int mask, int function) { int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE; - if (!ima_appraise) - flags = ~IMA_APPRAISE; + flags = ima_policy_flag; return ima_match_policy(inode, function, mask, flags); } diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 9164fc8..5e4c29d 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -24,6 +24,12 @@ #include crypto/hash_info.h #include ima.h +#ifdef CONFIG_IMA_X509_PATH +#define IMA_X509_PATH CONFIG_IMA_X509_PATH +#else +#define IMA_X509_PATH /etc/keys/x509_ima.der +#endif + /* name for boot aggregate entry */ static const char *boot_aggregate_name = boot_aggregate; int ima_used_chip; @@ -91,6 +97,17 @@ err_out: return result; } +#ifdef CONFIG_IMA_LOAD_X509 +void __init ima_load_x509(void) +{ + int unset_flags = ima_policy_flag IMA_APPRAISE; + + ima_policy_flag = ~unset_flags; + integrity_load_x509(INTEGRITY_KEYRING_IMA, IMA_X509_PATH); + ima_policy_flag |= unset_flags; +} +#endif + int __init ima_init(void) { u8 pcr_i[TPM_DIGEST_SIZE]; diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 1057abb..caa1f6c 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -162,6 +162,14 @@ static inline int asymmetric_verify(struct key *keyring, const char *sig, } #endif +#ifdef CONFIG_IMA_LOAD_X509 +void __init ima_load_x509(void); +#else +static inline void ima_load_x509(void) +{ +} +#endif + #ifdef CONFIG_INTEGRITY_AUDIT /* declarations */ void integrity_audit_msg(int audit_msgno, struct inode *inode, -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 6/6] VFS: refactor vfs_read()
integrity_kernel_read() duplicates the file read operations code in vfs_read(). This patch refactors vfs_read() code creating a helper function __vfs_read(). It is used by both vfs_read() and integrity_kernel_read(). Signed-off-by: Dmitry Kasatkin d.kasat...@samsung.com --- fs/read_write.c | 24 ++-- include/linux/fs.h| 1 + security/integrity/iint.c | 10 +++--- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/fs/read_write.c b/fs/read_write.c index 009d854..f45b2ae 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -412,6 +412,23 @@ ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, loff_t *p EXPORT_SYMBOL(new_sync_read); +ssize_t __vfs_read(struct file *file, char __user *buf, size_t count, + loff_t *pos) +{ + ssize_t ret; + + if (file-f_op-read) + ret = file-f_op-read(file, buf, count, pos); + else if (file-f_op-aio_read) + ret = do_sync_read(file, buf, count, pos); + else if (file-f_op-read_iter) + ret = new_sync_read(file, buf, count, pos); + else + ret = -EINVAL; + + return ret; +} + ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos) { ssize_t ret; @@ -426,12 +443,7 @@ ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos) ret = rw_verify_area(READ, file, pos, count); if (ret = 0) { count = ret; - if (file-f_op-read) - ret = file-f_op-read(file, buf, count, pos); - else if (file-f_op-aio_read) - ret = do_sync_read(file, buf, count, pos); - else - ret = new_sync_read(file, buf, count, pos); + ret = __vfs_read(file, buf, count, pos); if (ret 0) { fsnotify_access(file); add_rchar(current, ret); diff --git a/include/linux/fs.h b/include/linux/fs.h index e11d60c..ac3a36e 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1527,6 +1527,7 @@ ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector, struct iovec *fast_pointer, struct iovec **ret_pointer); +extern ssize_t __vfs_read(struct file *, char __user *, size_t, loff_t *); extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *); extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *); extern ssize_t vfs_readv(struct file *, const struct iovec __user *, diff --git a/security/integrity/iint.c b/security/integrity/iint.c index a1f5cd1..e359477 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -184,20 +184,16 @@ int integrity_kernel_read(struct file *file, loff_t offset, { mm_segment_t old_fs; char __user *buf = addr; - ssize_t ret = -EINVAL; + ssize_t ret; if (!(file-f_mode FMODE_READ)) return -EBADF; old_fs = get_fs(); set_fs(get_ds()); - if (file-f_op-read) - ret = file-f_op-read(file, buf, count, offset); - else if (file-f_op-aio_read) - ret = do_sync_read(file, buf, count, offset); - else if (file-f_op-read_iter) - ret = new_sync_read(file, buf, count, offset); + ret = __vfs_read(file, buf, count, offset); set_fs(old_fs); + return ret; } -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/