Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1421372 > unrolled thread

Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Andrew Zaborowski <balrogg@googlemail.com> - 2016-06-14 00:20 +0200
    Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-14 07:20 +0200
      Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Andrew Zaborowski <balrogg@googlemail.com> - 2016-06-14 09:50 +0200
        Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-16 10:10 +0200
          Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Andrew Zaborowski <balrogg@googlemail.com> - 2016-06-16 17:00 +0200
            Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Stephan Mueller <smueller@chronox.de> - 2016-06-16 17:40 +0200
              Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface Andrew Zaborowski <balrogg@googlemail.com> - 2016-06-17 02:50 +0200

#1421372 — Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface

Hi,

On 8 June 2016 at 21:14, Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
> On Wed, 8 Jun 2016, Stephan Mueller wrote:
>> What is your concern?
> Userspace must allocate larger buffers than it knows are necessary for
> expected results.
>
> It looks like the software rsa implementation handles shorter output buffers
> ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is too small),
> however I see at least one hardware rsa driver that requires the output
> buffer to be the maximum size. But this inconsistency might be best
> addressed within the software cipher or drivers rather than in recvmsg.

Should the hardware drivers fix this instead?  I've looked at the qat
and caam drivers, they both require the destination buffer size to be
the key size and in both cases there would be no penalty for dropping
this requirement as far as I see.  Both do a memmove if the result
ends up being shorter than key size.  In case the caller knows it is
expecting a specific output size, the driver will have to use a self
allocated buffer + a memcpy in those same cases where it would later
use memmove instead.  Alternatively the sg passed to dma_map_sg can be
prepended with a dummy segment the right size to save the memcpy.

akcipher.h only says:
@dst_len: Size of the output buffer. It needs to be at least as big as
the expected result depending on the operation

Note that for random input data the memmove will be done about 1 in
256 times but with PKCS#1 padding the signature always has a leading
zero.

Requiring buffers bigger than needed makes the added work of dropping
the zero bytes from the sglist and potentially re-adding them in the
client difficult to justify.  RSA doing this sets a precedent for a
future pkcs1pad (or other algorithm) implementation to do the same
thing and a portable client having to always know the key size and use
key-sized buffers.

Best regards

[toc] | [next] | [standalone]

#1421580

Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski:

Hi Andrew,

> Hi,
> 
> On 8 June 2016 at 21:14, Mat Martineau
> 
> <mathew.j.martineau@linux.intel.com> wrote:
> > On Wed, 8 Jun 2016, Stephan Mueller wrote:
> >> What is your concern?
> > 
> > Userspace must allocate larger buffers than it knows are necessary for
> > expected results.
> > 
> > It looks like the software rsa implementation handles shorter output
> > buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
> > too small), however I see at least one hardware rsa driver that requires
> > the output buffer to be the maximum size. But this inconsistency might be
> > best addressed within the software cipher or drivers rather than in
> > recvmsg.
> Should the hardware drivers fix this instead?  I've looked at the qat
> and caam drivers, they both require the destination buffer size to be
> the key size and in both cases there would be no penalty for dropping
> this requirement as far as I see.  Both do a memmove if the result
> ends up being shorter than key size.  In case the caller knows it is
> expecting a specific output size, the driver will have to use a self
> allocated buffer + a memcpy in those same cases where it would later
> use memmove instead.  Alternatively the sg passed to dma_map_sg can be
> prepended with a dummy segment the right size to save the memcpy.
> 
> akcipher.h only says:
> @dst_len: Size of the output buffer. It needs to be at least as big as
> the expected result depending on the operation
> 
> Note that for random input data the memmove will be done about 1 in
> 256 times but with PKCS#1 padding the signature always has a leading
> zero.
> 
> Requiring buffers bigger than needed makes the added work of dropping
> the zero bytes from the sglist and potentially re-adding them in the
> client difficult to justify.  RSA doing this sets a precedent for a
> future pkcs1pad (or other algorithm) implementation to do the same
> thing and a portable client having to always know the key size and use
> key-sized buffers.

I think we have agreed on dropping the length enforcement at the interface 
level.

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1421634

Hi Stephan,

On 14 June 2016 at 07:12, Stephan Mueller <smueller@chronox.de> wrote:
> Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski:
>> On 8 June 2016 at 21:14, Mat Martineau
>>
>> <mathew.j.martineau@linux.intel.com> wrote:
>> > On Wed, 8 Jun 2016, Stephan Mueller wrote:
>> >> What is your concern?
>> >
>> > Userspace must allocate larger buffers than it knows are necessary for
>> > expected results.
>> >
>> > It looks like the software rsa implementation handles shorter output
>> > buffers ok (mpi_write_to_sgl will return EOVERFLOW if the the buffer is
>> > too small), however I see at least one hardware rsa driver that requires
>> > the output buffer to be the maximum size. But this inconsistency might be
>> > best addressed within the software cipher or drivers rather than in
>> > recvmsg.
>> Should the hardware drivers fix this instead?  I've looked at the qat
>> and caam drivers, they both require the destination buffer size to be
>> the key size and in both cases there would be no penalty for dropping
>> this requirement as far as I see.  Both do a memmove if the result
>> ends up being shorter than key size.  In case the caller knows it is
>> expecting a specific output size, the driver will have to use a self
>> allocated buffer + a memcpy in those same cases where it would later
>> use memmove instead.  Alternatively the sg passed to dma_map_sg can be
>> prepended with a dummy segment the right size to save the memcpy.
>>
>> akcipher.h only says:
>> @dst_len: Size of the output buffer. It needs to be at least as big as
>> the expected result depending on the operation
>>
>> Note that for random input data the memmove will be done about 1 in
>> 256 times but with PKCS#1 padding the signature always has a leading
>> zero.
>>
>> Requiring buffers bigger than needed makes the added work of dropping
>> the zero bytes from the sglist and potentially re-adding them in the
>> client difficult to justify.  RSA doing this sets a precedent for a
>> future pkcs1pad (or other algorithm) implementation to do the same
>> thing and a portable client having to always know the key size and use
>> key-sized buffers.
>
> I think we have agreed on dropping the length enforcement at the interface
> level.

Separately from this there's a problem with the user being unable to
know if the algorithm is going to fail because of destination buffer
size != key size (including kernel users).  For RSA, the qat
implementation will fail while the software implementation won't.  For
pkcs1pad(...) there's currently just one implementation but the user
can't assume that.

Best regards

[toc] | [prev] | [next] | [standalone]

#1423768

Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:

Hi Andrew,

> > 
> > I think we have agreed on dropping the length enforcement at the interface
> > level.
> 
> Separately from this there's a problem with the user being unable to
> know if the algorithm is going to fail because of destination buffer
> size != key size (including kernel users).  For RSA, the qat
> implementation will fail while the software implementation won't.  For
> pkcs1pad(...) there's currently just one implementation but the user
> can't assume that.

If I understand your issue correctly, my initial code requiring the caller to 
provide sufficient memory would have covered the issue, right? If so, we seem 
to have implementations which can handle shorter buffer sizes and some which 
do not. Should a caller really try to figure the right buffer size out? Why 
not requiring a mandatory buffer size and be done with it? I.e. what is the 
gain to allow shorter buffer sizes (as pointed out by Mat)? So, bottom line, I 
am wondering whether we should keep the algif_akcipher code to require a 
minimum buffer size.

If there is really a good argument to allow shorter buffers, then I guess we 
need an in-kernel API call (which should be reported to user space) which 
gives us the smallest usable buffer size. I guess that call would only be 
valid after a setkey operation as the output size depends on the key size. 
Instead of inventing a complete new API call, shouldn't the call 
crypto_akcipher_maxsize() be converted for this purpose? I requested that API 
call during the time the akcipher API was developed explicitly for getting the 
minimum buffer size the caller needs to provide.

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1424152

Hi Stephan,

On 16 June 2016 at 10:05, Stephan Mueller <smueller@chronox.de> wrote:
> Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:
>
> Hi Andrew,
>
>> >
>> > I think we have agreed on dropping the length enforcement at the interface
>> > level.
>>
>> Separately from this there's a problem with the user being unable to
>> know if the algorithm is going to fail because of destination buffer
>> size != key size (including kernel users).  For RSA, the qat
>> implementation will fail while the software implementation won't.  For
>> pkcs1pad(...) there's currently just one implementation but the user
>> can't assume that.
>
> If I understand your issue correctly, my initial code requiring the caller to
> provide sufficient memory would have covered the issue, right?

This isn't an issue with AF_ALG, I should have changed the subject
line perhaps.  In this case it's an inconsistency between some
implementations and the documentation (header comment).  It affects
users accessing the cipher through AF_ALG but also directly.

> If so, we seem
> to have implementations which can handle shorter buffer sizes and some which
> do not. Should a caller really try to figure the right buffer size out? Why
> not requiring a mandatory buffer size and be done with it? I.e. what is the
> gain to allow shorter buffer sizes (as pointed out by Mat)?

It's that client code doesn't need an intermediate layer with an
additional buffer and a memcpy to provide a sensible API.  If the code
wants to decrypt a 32-byte Digest Info structure with a given key or a
reference to a key it makes no sense, logically or in terms of
performance, for it to provide a key-sized buffer.

In the case of the userspace interface I think it's also rare for a
recv() or read() on Linux to require a buffer larger than it's going
to use, correct me if i'm wrong.  (I.e. fail if given a 32-byte
buffer, return 32 bytes of data anyway)  Turning your questino around
is there a gain from requiring larger buffers?

Best regards

[toc] | [prev] | [next] | [standalone]

#1424200

Am Donnerstag, 16. Juni 2016, 16:59:01 schrieb Andrew Zaborowski:

Hi Andrew,

> Hi Stephan,
> 
> On 16 June 2016 at 10:05, Stephan Mueller <smueller@chronox.de> wrote:
> > Am Dienstag, 14. Juni 2016, 09:42:34 schrieb Andrew Zaborowski:
> > 
> > Hi Andrew,
> > 
> >> > I think we have agreed on dropping the length enforcement at the
> >> > interface
> >> > level.
> >> 
> >> Separately from this there's a problem with the user being unable to
> >> know if the algorithm is going to fail because of destination buffer
> >> size != key size (including kernel users).  For RSA, the qat
> >> implementation will fail while the software implementation won't.  For
> >> pkcs1pad(...) there's currently just one implementation but the user
> >> can't assume that.
> > 
> > If I understand your issue correctly, my initial code requiring the caller
> > to provide sufficient memory would have covered the issue, right?
> 
> This isn't an issue with AF_ALG, I should have changed the subject
> line perhaps.  In this case it's an inconsistency between some
> implementations and the documentation (header comment).  It affects
> users accessing the cipher through AF_ALG but also directly.

As I want to send a new version of the algif_akcipher shortly now (hoping for 
an inclusion into 4.8), is there anything you see that I should prepare for 
regarding this issue? I.e. do you forsee a potential fix that would change the 
API or ABI of algif_akcipher?
> 
> > If so, we seem
> > to have implementations which can handle shorter buffer sizes and some
> > which do not. Should a caller really try to figure the right buffer size
> > out? Why not requiring a mandatory buffer size and be done with it? I.e.
> > what is the gain to allow shorter buffer sizes (as pointed out by Mat)?
> 
> It's that client code doesn't need an intermediate layer with an
> additional buffer and a memcpy to provide a sensible API.  If the code
> wants to decrypt a 32-byte Digest Info structure with a given key or a
> reference to a key it makes no sense, logically or in terms of
> performance, for it to provide a key-sized buffer.
> 
> In the case of the userspace interface I think it's also rare for a
> recv() or read() on Linux to require a buffer larger than it's going
> to use, correct me if i'm wrong.  (I.e. fail if given a 32-byte
> buffer, return 32 bytes of data anyway)  Turning your questino around
> is there a gain from requiring larger buffers?

That is a good one :-)

I have that check removed.

Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1424569

Hi Stephan,

On 16 June 2016 at 17:38, Stephan Mueller <smueller@chronox.de> wrote:
>> This isn't an issue with AF_ALG, I should have changed the subject
>> line perhaps.  In this case it's an inconsistency between some
>> implementations and the documentation (header comment).  It affects
>> users accessing the cipher through AF_ALG but also directly.
>
> As I want to send a new version of the algif_akcipher shortly now (hoping for
> an inclusion into 4.8), is there anything you see that I should prepare for
> regarding this issue? I.e. do you forsee a potential fix that would change the
> API or ABI of algif_akcipher?

No, as far as I understand algif_akcipher will do the right thing now
if the algorithm does the right thing.  It's only the two RSA drivers
that would need to align with the software RSA in what buffer length
they accept.

Best regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web