diff mbox series

[v5,2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature()

Message ID 20221227142740.2807136-3-roberto.sassu@huaweicloud.com
State New
Headers
Series KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() |

Commit Message

Roberto Sassu Dec. 27, 2022, 2:27 p.m. UTC 
  From: Roberto Sassu <roberto.sassu@huawei.com>

Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
adjacent pages.

Always make a copy of the signature and digest in the same buffer used to
store the key and its parameters, and pass them to sg_init_one(). Prefer it
to conditionally doing the copy if necessary, to keep the code simple. The
buffer allocated with kmalloc() is in the linear mapping area.

Cc: stable@vger.kernel.org # 4.9.x
Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
Suggested-by: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Eric Biggers Dec. 29, 2022, 10:39 p.m. UTC | #1 
On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> adjacent pages.
> 
> Always make a copy of the signature and digest in the same buffer used to
> store the key and its parameters, and pass them to sg_init_one(). Prefer it
> to conditionally doing the copy if necessary, to keep the code simple. The
> buffer allocated with kmalloc() is in the linear mapping area.
> 
> Cc: stable@vger.kernel.org # 4.9.x
> Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> Suggested-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 17 deletions(-)

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
Roberto Sassu Jan. 27, 2023, 8:27 a.m. UTC | #2 
On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > adjacent pages.
> > 
> > Always make a copy of the signature and digest in the same buffer used to
> > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > to conditionally doing the copy if necessary, to keep the code simple. The
> > buffer allocated with kmalloc() is in the linear mapping area.
> > 
> > Cc: stable@vger.kernel.org # 4.9.x
> > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> >  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Hi David

could you please take this patch in your repo, if it is ok?

Thanks

Roberto
Roberto Sassu Feb. 9, 2023, 10:49 a.m. UTC | #3 
On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > adjacent pages.
> > > 
> > > Always make a copy of the signature and digest in the same buffer used to
> > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > buffer allocated with kmalloc() is in the linear mapping area.
> > > 
> > > Cc: stable@vger.kernel.org # 4.9.x
> > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > Reviewed-by: Eric Biggers <ebiggers@google.com>
> 
> Hi David
> 
> could you please take this patch in your repo, if it is ok?

Kindly ask your support here. Has this patch been queued somewhere?
Wasn't able to find it, also it is not in linux-next.

Thanks

Roberto
Eric Biggers Feb. 9, 2023, 6:53 p.m. UTC | #4 
On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > adjacent pages.
> > > > 
> > > > Always make a copy of the signature and digest in the same buffer used to
> > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > 
> > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > 
> > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > 
> > Hi David
> > 
> > could you please take this patch in your repo, if it is ok?
> 
> Kindly ask your support here. Has this patch been queued somewhere?
> Wasn't able to find it, also it is not in linux-next.
> 

The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
reason; should that change?).

- Eric
Paul Moore Feb. 9, 2023, 10:46 p.m. UTC | #5 
On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote:
> On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > >
> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > adjacent pages.
> > > > >
> > > > > Always make a copy of the signature and digest in the same buffer used to
> > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > >
> > > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > >
> > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > >
> > > Hi David
> > >
> > > could you please take this patch in your repo, if it is ok?
> >
> > Kindly ask your support here. Has this patch been queued somewhere?
> > Wasn't able to find it, also it is not in linux-next.
> >
>
> The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
> you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
> crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
> is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
> reason; should that change?).

It is problematic that David isn't replying to this.  I have no idea
if it will work, but I just reached out to him to see if I can draw
his attention back to this ...

--
paul-moore.com
Jarkko Sakkinen Feb. 10, 2023, 3:52 a.m. UTC | #6 
On Thu, Feb 09, 2023 at 06:53:37PM +0000, Eric Biggers wrote:
> On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > adjacent pages.
> > > > > 
> > > > > Always make a copy of the signature and digest in the same buffer used to
> > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > > 
> > > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > 
> > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > 
> > > Hi David
> > > 
> > > could you please take this patch in your repo, if it is ok?
> > 
> > Kindly ask your support here. Has this patch been queued somewhere?
> > Wasn't able to find it, also it is not in linux-next.
> > 
> 
> The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
> you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
> crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
> is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
> reason; should that change?).

I can apply this if no objections?

BR, Jarkko
Jarkko Sakkinen Feb. 10, 2023, 3:56 a.m. UTC | #7 
On Thu, Feb 09, 2023 at 05:46:32PM -0500, Paul Moore wrote:
> On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > >
> > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > > adjacent pages.
> > > > > >
> > > > > > Always make a copy of the signature and digest in the same buffer used to
> > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > >
> > > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > >
> > > > Hi David
> > > >
> > > > could you please take this patch in your repo, if it is ok?
> > >
> > > Kindly ask your support here. Has this patch been queued somewhere?
> > > Wasn't able to find it, also it is not in linux-next.
> > >
> >
> > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
> > you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
> > crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
> > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
> > reason; should that change?).
> 
> It is problematic that David isn't replying to this.  I have no idea
> if it will work, but I just reached out to him to see if I can draw
> his attention back to this ...

See my response to Eric.

BR, Jarkko
Paul Moore Feb. 10, 2023, 5:32 p.m. UTC | #8 
On Thu, Feb 9, 2023 at 10:56 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> On Thu, Feb 09, 2023 at 05:46:32PM -0500, Paul Moore wrote:
> > On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> > > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > >
> > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > > > adjacent pages.
> > > > > > >
> > > > > > > Always make a copy of the signature and digest in the same buffer used to
> > > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > > > >
> > > > > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > ---
> > > > > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > > >
> > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > > >
> > > > > Hi David
> > > > >
> > > > > could you please take this patch in your repo, if it is ok?
> > > >
> > > > Kindly ask your support here. Has this patch been queued somewhere?
> > > > Wasn't able to find it, also it is not in linux-next.
> > > >
> > >
> > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
> > > you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
> > > crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
> > > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
> > > reason; should that change?).
> >
> > It is problematic that David isn't replying to this.  I have no idea
> > if it will work, but I just reached out to him to see if I can draw
> > his attention back to this ...
>
> See my response to Eric.

Thanks Jarkko.
Roberto Sassu May 26, 2023, 7:35 a.m. UTC | #9 
On Fri, 2023-02-10 at 05:52 +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 09, 2023 at 06:53:37PM +0000, Eric Biggers wrote:
> > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote:
> > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote:
> > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote:
> > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote:
> > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > 
> > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > > > adjacent pages.
> > > > > > 
> > > > > > Always make a copy of the signature and digest in the same buffer used to
> > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > > > 
> > > > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > >  crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> > > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > > 
> > > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > Hi David
> > > > 
> > > > could you please take this patch in your repo, if it is ok?
> > > 
> > > Kindly ask your support here. Has this patch been queued somewhere?
> > > Wasn't able to find it, also it is not in linux-next.
> > > 
> > 
> > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so
> > you'll need to find someone else to apply it.  Herbert Xu, the maintainer of the
> > crypto subsystem, might be willing to apply it.  Or maybe Jarkko Sakkinen, who
> > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some
> > reason; should that change?).
> 
> I can apply this if no objections?

Hi Jarkko

I wasn't able to reach David about this patch. Could you please apply
it?

Thanks

Roberto
Stefan Berger June 1, 2023, 9 p.m. UTC | #10 
On 12/27/22 09:27, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> adjacent pages.
> 
> Always make a copy of the signature and digest in the same buffer used to
> store the key and its parameters, and pass them to sg_init_one(). Prefer it
> to conditionally doing the copy if necessary, to keep the code simple. The
> buffer allocated with kmalloc() is in the linear mapping area.
> 
> Cc: stable@vger.kernel.org # 4.9.x
> Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> Suggested-by: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>

I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue.

Tested-by: Stefan Berger <stefanb@linux.ibm.com>



> ---
>   crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 2f8352e88860..49a3f7c01149 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey,
>   	struct crypto_wait cwait;
>   	struct crypto_akcipher *tfm;
>   	struct akcipher_request *req;
> -	struct scatterlist src_sg[2];
> +	struct scatterlist src_sg;
>   	char alg_name[CRYPTO_MAX_ALG_NAME];
> -	char *key, *ptr;
> +	char *buf, *ptr;
> +	size_t buf_len;
>   	int ret;
>   
>   	pr_devel("==>%s()\n", __func__);
> @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey,
>   	if (!req)
>   		goto error_free_tfm;
>   
> -	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> -		      GFP_KERNEL);
> -	if (!key)
> +	buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> +			sig->s_size + sig->digest_size);
> +
> +	buf = kmalloc(buf_len, GFP_KERNEL);
> +	if (!buf)
>   		goto error_free_req;
>   
> -	memcpy(key, pkey->key, pkey->keylen);
> -	ptr = key + pkey->keylen;
> +	memcpy(buf, pkey->key, pkey->keylen);
> +	ptr = buf + pkey->keylen;
>   	ptr = pkey_pack_u32(ptr, pkey->algo);
>   	ptr = pkey_pack_u32(ptr, pkey->paramlen);
>   	memcpy(ptr, pkey->params, pkey->paramlen);
>   
>   	if (pkey->key_is_private)
> -		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
> +		ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen);
>   	else
> -		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
> +		ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen);
>   	if (ret)
> -		goto error_free_key;
> +		goto error_free_buf;
>   
>   	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
>   		ret = cert_sig_digest_update(sig, tfm);
>   		if (ret)
> -			goto error_free_key;
> +			goto error_free_buf;
>   	}
>   
> -	sg_init_table(src_sg, 2);
> -	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
> -	sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
> -	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
> +	memcpy(buf, sig->s, sig->s_size);
> +	memcpy(buf + sig->s_size, sig->digest, sig->digest_size);
> +
> +	sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size);
> +	akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size,
>   				   sig->digest_size);
>   	crypto_init_wait(&cwait);
>   	akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey,
>   				      crypto_req_done, &cwait);
>   	ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait);
>   
> -error_free_key:
> -	kfree(key);
> +error_free_buf:
> +	kfree(buf);
>   error_free_req:
>   	akcipher_request_free(req);
>   error_free_tfm:
Roberto Sassu June 2, 2023, 9:17 a.m. UTC | #11 
On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote:
> 
> On 12/27/22 09:27, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > adjacent pages.
> > 
> > Always make a copy of the signature and digest in the same buffer used to
> > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > to conditionally doing the copy if necessary, to keep the code simple. The
> > buffer allocated with kmalloc() is in the linear mapping area.
> > 
> > Cc: stable@vger.kernel.org # 4.9.x
> > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Eric Biggers <ebiggers@google.com>
> 
> I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue.
> 
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks, Stefan.

I did multiple attempts to have the patch included, but I didn't have
any luck with the maintainers (David, Jarkko).

It would be awesome if any maintainer picks it.

Thanks!

Roberto

> ---
> >   crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++-------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> > index 2f8352e88860..49a3f7c01149 100644
> > --- a/crypto/asymmetric_keys/public_key.c
> > +++ b/crypto/asymmetric_keys/public_key.c
> > @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   	struct crypto_wait cwait;
> >   	struct crypto_akcipher *tfm;
> >   	struct akcipher_request *req;
> > -	struct scatterlist src_sg[2];
> > +	struct scatterlist src_sg;
> >   	char alg_name[CRYPTO_MAX_ALG_NAME];
> > -	char *key, *ptr;
> > +	char *buf, *ptr;
> > +	size_t buf_len;
> >   	int ret;
> >   
> >   	pr_devel("==>%s()\n", __func__);
> > @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   	if (!req)
> >   		goto error_free_tfm;
> >   
> > -	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> > -		      GFP_KERNEL);
> > -	if (!key)
> > +	buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> > +			sig->s_size + sig->digest_size);
> > +
> > +	buf = kmalloc(buf_len, GFP_KERNEL);
> > +	if (!buf)
> >   		goto error_free_req;
> >   
> > -	memcpy(key, pkey->key, pkey->keylen);
> > -	ptr = key + pkey->keylen;
> > +	memcpy(buf, pkey->key, pkey->keylen);
> > +	ptr = buf + pkey->keylen;
> >   	ptr = pkey_pack_u32(ptr, pkey->algo);
> >   	ptr = pkey_pack_u32(ptr, pkey->paramlen);
> >   	memcpy(ptr, pkey->params, pkey->paramlen);
> >   
> >   	if (pkey->key_is_private)
> > -		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
> > +		ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen);
> >   	else
> > -		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
> > +		ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen);
> >   	if (ret)
> > -		goto error_free_key;
> > +		goto error_free_buf;
> >   
> >   	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
> >   		ret = cert_sig_digest_update(sig, tfm);
> >   		if (ret)
> > -			goto error_free_key;
> > +			goto error_free_buf;
> >   	}
> >   
> > -	sg_init_table(src_sg, 2);
> > -	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
> > -	sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
> > -	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
> > +	memcpy(buf, sig->s, sig->s_size);
> > +	memcpy(buf + sig->s_size, sig->digest, sig->digest_size);
> > +
> > +	sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size);
> > +	akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size,
> >   				   sig->digest_size);
> >   	crypto_init_wait(&cwait);
> >   	akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> > @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey,
> >   				      crypto_req_done, &cwait);
> >   	ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait);
> >   
> > -error_free_key:
> > -	kfree(key);
> > +error_free_buf:
> > +	kfree(buf);
> >   error_free_req:
> >   	akcipher_request_free(req);
> >   error_free_tfm:
Eric Biggers June 2, 2023, 1:17 p.m. UTC | #12 
On Fri, Jun 02, 2023 at 11:17:04AM +0200, Roberto Sassu wrote:
> On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote:
> > 
> > On 12/27/22 09:27, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > adjacent pages.
> > > 
> > > Always make a copy of the signature and digest in the same buffer used to
> > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > buffer allocated with kmalloc() is in the linear mapping area.
> > > 
> > > Cc: stable@vger.kernel.org # 4.9.x
> > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > 
> > I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue.
> > 
> > Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Thanks, Stefan.
> 
> I did multiple attempts to have the patch included, but I didn't have
> any luck with the maintainers (David, Jarkko).
> 
> It would be awesome if any maintainer picks it.
> 
> Thanks!
> 

As the maintainers are ignoring this patch, you could try the "maintainers of
last resort" (Andrew Morton or Linus Torvalds).

- Eric
Roberto Sassu June 2, 2023, 1:39 p.m. UTC | #13 
On Fri, 2023-06-02 at 06:17 -0700, Eric Biggers wrote:
> On Fri, Jun 02, 2023 at 11:17:04AM +0200, Roberto Sassu wrote:
> > On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote:
> > > On 12/27/22 09:27, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear
> > > > mapping") checks that both the signature and the digest reside 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 is not contiguous, and thus not suitable for sg_set_buf() which needs
> > > > adjacent pages.
> > > > 
> > > > Always make a copy of the signature and digest in the same buffer used to
> > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it
> > > > to conditionally doing the copy if necessary, to keep the code simple. The
> > > > buffer allocated with kmalloc() is in the linear mapping area.
> > > > 
> > > > Cc: stable@vger.kernel.org # 4.9.x
> > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support")
> > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/
> > > > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: Eric Biggers <ebiggers@google.com>
> > > 
> > > I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue.
> > > 
> > > Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > Thanks, Stefan.
> > 
> > I did multiple attempts to have the patch included, but I didn't have
> > any luck with the maintainers (David, Jarkko).
> > 
> > It would be awesome if any maintainer picks it.
> > 
> > Thanks!
> > 
> 
> As the maintainers are ignoring this patch, you could try the "maintainers of
> last resort" (Andrew Morton or Linus Torvalds).

Thanks, will do.

Roberto
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index 2f8352e88860..49a3f7c01149 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -360,9 +360,10 @@  int public_key_verify_signature(const struct public_key *pkey,
 	struct crypto_wait cwait;
 	struct crypto_akcipher *tfm;
 	struct akcipher_request *req;
-	struct scatterlist src_sg[2];
+	struct scatterlist src_sg;
 	char alg_name[CRYPTO_MAX_ALG_NAME];
-	char *key, *ptr;
+	char *buf, *ptr;
+	size_t buf_len;
 	int ret;
 
 	pr_devel("==>%s()\n", __func__);
@@ -400,34 +401,37 @@  int public_key_verify_signature(const struct public_key *pkey,
 	if (!req)
 		goto error_free_tfm;
 
-	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
-		      GFP_KERNEL);
-	if (!key)
+	buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
+			sig->s_size + sig->digest_size);
+
+	buf = kmalloc(buf_len, GFP_KERNEL);
+	if (!buf)
 		goto error_free_req;
 
-	memcpy(key, pkey->key, pkey->keylen);
-	ptr = key + pkey->keylen;
+	memcpy(buf, pkey->key, pkey->keylen);
+	ptr = buf + pkey->keylen;
 	ptr = pkey_pack_u32(ptr, pkey->algo);
 	ptr = pkey_pack_u32(ptr, pkey->paramlen);
 	memcpy(ptr, pkey->params, pkey->paramlen);
 
 	if (pkey->key_is_private)
-		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
+		ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen);
 	else
-		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
+		ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen);
 	if (ret)
-		goto error_free_key;
+		goto error_free_buf;
 
 	if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) {
 		ret = cert_sig_digest_update(sig, tfm);
 		if (ret)
-			goto error_free_key;
+			goto error_free_buf;
 	}
 
-	sg_init_table(src_sg, 2);
-	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
-	sg_set_buf(&src_sg[1], sig->digest, sig->digest_size);
-	akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size,
+	memcpy(buf, sig->s, sig->s_size);
+	memcpy(buf + sig->s_size, sig->digest, sig->digest_size);
+
+	sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size);
+	akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size,
 				   sig->digest_size);
 	crypto_init_wait(&cwait);
 	akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
@@ -435,8 +439,8 @@  int public_key_verify_signature(const struct public_key *pkey,
 				      crypto_req_done, &cwait);
 	ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait);
 
-error_free_key:
-	kfree(key);
+error_free_buf:
+	kfree(buf);
 error_free_req:
 	akcipher_request_free(req);
 error_free_tfm: