Re: [PATCH] ima: Make a copy of sig and digest in asymmetric_verify()

[Roberto Sassu]

On Wed, 2022-11-23 at 14:49 +0100, Roberto Sassu wrote:
> On Wed, 2022-11-23 at 08:40 -0500, Mimi Zohar wrote:
> > On Wed, 2022-11-23 at 13:56 +0100, Roberto Sassu wrote:
> > > On Tue, 2022-11-22 at 14:39 -0500, Mimi Zohar wrote:
> > > > Hi Roberto,
> > > > 
> > > > On Fri, 2022-11-04 at 13:20 +0100, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > > 
> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > mapping") requires that both the signature and the digest resides in the
> > > > > linear mapping area.
> > > > > 
> > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced
> > > > > stack support"), made it possible to move the stack in the vmalloc area,
> > > > > which could make the requirement of the first commit not satisfied anymore.
> > > > > 
> > > > > If CONFIG_SG=y and CONFIG_VMAP_STACK=y, the following BUG() is triggered:
> > > > 
> > > > ^CONFIG_DEBUG_SG
> > > > 
> > > > > [  467.077359] kernel BUG at include/linux/scatterlist.h:163!
> > > > > [  467.077939] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> > > > > 
> > > > > [...]
> > > > > 
> > > > > [  467.095225] Call Trace:
> > > > > [  467.096088]  <TASK>
> > > > > [  467.096928]  ? rcu_read_lock_held_common+0xe/0x50
> > > > > [  467.097569]  ? rcu_read_lock_sched_held+0x13/0x70
> > > > > [  467.098123]  ? trace_hardirqs_on+0x2c/0xd0
> > > > > [  467.098647]  ? public_key_verify_signature+0x470/0x470
> > > > > [  467.099237]  asymmetric_verify+0x14c/0x300
> > > > > [  467.099869]  evm_verify_hmac+0x245/0x360
> > > > > [  467.100391]  evm_inode_setattr+0x43/0x190
> > > > > 
> > > > > The failure happens only for the digest, as the pointer comes from the
> > > > > stack, and not for the signature, which instead was allocated by
> > > > > vfs_getxattr_alloc().
> > > > 
> > > > Only after enabling CONFIG_DEBUG_SG does EVM fail.
> > > > 
> > > > > Fix this by making a copy of both in asymmetric_verify(), so that the
> > > > > linear mapping requirement is always satisfied, regardless of the caller.
> > > > 
> > > > As only EVM is affected, it would make more sense to limit the change
> > > > to EVM.
> > > 
> > > I found another occurrence:
> > > 
> > > static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
> > > 			struct evm_ima_xattr_data *xattr_value, int xattr_len,
> > > 			enum integrity_status *status, const char **cause)
> > > {
> > > 
> > > [...]
> > > 
> > > 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> > > 					     (const char *)xattr_value,
> > > 					     xattr_len, hash.digest,
> > > 					     hash.hdr.length);
> > > 
> > > Should I do two patches?
> > 
> > I'm just not getting it.  Why did you enable CONFIG_DEBUG_SIG?  Were
> > you testing random kernel configs?  Are you actually seeing signature
> > verifications errors without it enabled?  Or is it causing other
> > problems?  Is the "BUG_ON" still needed?
> 
> When I test patches, I tend to enable more debugging options.
> 
> To be honest, I didn't check if there is any issue without enabling
> CONFIG_DEBUG_SG. I thought that if there is a linear mapping
> requirement, that should be satisfied regardless of whether the
> debugging option is enabled or not.
> 
> + Rusty, Jens for explanations.

Trying to answer the question, with the help of an old discussion:

https://groups.google.com/g/linux.kernel/c/dpIoiY_qSGc

sg_set_buf() calls virt_to_page() to get the page to start from. But if
the buffer spans in two pages, that would not work in the vmalloc area,
since there is no guarantee that the next page is adjiacent.

For small areas, much smaller than the page size, it is unlikely that
the situation above would happen. So, integrity_digsig_verify() will
likely succeeed. Although it is possible that it fails if there are
data in the next page.

Roberto