Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Stephan, On Sat, 14 May 2016, Tadeusz Struk wrote: From: Stephan Mueller This patch adds the user space interface for asymmetric ciphers. The interface allows the use of sendmsg as well as vmsplice to provide data. This version has been rebased on top of 4.6 and a few chackpatch issues have been fixed. Signed-off-by: Stephan Mueller Signed-off-by: Tadeusz Struk --- diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c new file mode 100644 index 000..6342b6e --- /dev/null +++ b/crypto/algif_akcipher.c + +static int akcipher_recvmsg(struct socket *sock, struct msghdr *msg, + size_t ignored, int flags) +{ + struct sock *sk = sock->sk; + struct alg_sock *ask = alg_sk(sk); + struct akcipher_ctx *ctx = ask->private; + struct akcipher_sg_list *sgl = &ctx->tsgl; + unsigned int i = 0; + int err; + unsigned long used = 0; + size_t usedpages = 0; + unsigned int cnt = 0; + + /* Limit number of IOV blocks to be accessed below */ + if (msg->msg_iter.nr_segs > ALG_MAX_PAGES) + return -ENOMSG; + + lock_sock(sk); + + if (ctx->more) { + err = akcipher_wait_for_data(sk, flags); + if (err) + goto unlock; + } + + used = ctx->used; + + /* convert iovecs of output buffers into scatterlists */ + while (iov_iter_count(&msg->msg_iter)) { + /* make one iovec available as scatterlist */ + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter, +iov_iter_count(&msg->msg_iter)); + if (err < 0) + goto unlock; + usedpages += err; + /* chain the new scatterlist with previous one */ + if (cnt) + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]); + + iov_iter_advance(&msg->msg_iter, err); + cnt++; + } + + /* ensure output buffer is sufficiently large */ + if (usedpages < akcipher_calcsize(ctx)) { + err = -EMSGSIZE; + goto unlock; + } Why is the size of the output buffer enforced here instead of depending on the algorithm implementation? Thanks, Mat + sg_mark_end(sgl->sg + sgl->cur - 1); + + akcipher_request_set_crypt(&ctx->req, sgl->sg, ctx->rsgl[0].sg, used, + usedpages); + switch (ctx->op) { + case ALG_OP_VERIFY: + err = crypto_akcipher_verify(&ctx->req); + break; + case ALG_OP_SIGN: + err = crypto_akcipher_sign(&ctx->req); + break; + case ALG_OP_ENCRYPT: + err = crypto_akcipher_encrypt(&ctx->req); + break; + case ALG_OP_DECRYPT: + err = crypto_akcipher_decrypt(&ctx->req); + break; + default: + err = -EFAULT; + goto unlock; + } + + err = af_alg_wait_for_completion(err, &ctx->completion); + + if (err) { + /* EBADMSG implies a valid cipher operation took place */ + if (err == -EBADMSG) + akcipher_put_sgl(sk); + goto unlock; + } + + akcipher_put_sgl(sk); + +unlock: + for (i = 0; i < cnt; i++) + af_alg_free_sg(&ctx->rsgl[i]); + + akcipher_wmem_wakeup(sk); + release_sock(sk); + + return err ? err : ctx->req.dst_len; +} -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau: Hi Mat, > > + used = ctx->used; > > + > > + /* convert iovecs of output buffers into scatterlists */ > > + while (iov_iter_count(&msg->msg_iter)) { > > + /* make one iovec available as scatterlist */ > > + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter, > > +iov_iter_count(&msg->msg_iter)); > > + if (err < 0) > > + goto unlock; > > + usedpages += err; > > + /* chain the new scatterlist with previous one */ > > + if (cnt) > > + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]); > > + > > + iov_iter_advance(&msg->msg_iter, err); > > + cnt++; > > + } > > + > > + /* ensure output buffer is sufficiently large */ > > + if (usedpages < akcipher_calcsize(ctx)) { > > + err = -EMSGSIZE; > > + goto unlock; > > + } > > Why is the size of the output buffer enforced here instead of depending on > the algorithm implementation? akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the algorithm generates as output during its operation. The code ensures that the caller provided at least that amount of memory for the kernel to store its data in. This check therefore is present to ensure the kernel does not overstep memory boundaries in user space. What is your concern? Thanks Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
On Wed, 8 Jun 2016, Stephan Mueller wrote: Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau: Hi Mat, + used = ctx->used; + + /* convert iovecs of output buffers into scatterlists */ + while (iov_iter_count(&msg->msg_iter)) { + /* make one iovec available as scatterlist */ + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter, +iov_iter_count(&msg->msg_iter)); + if (err < 0) + goto unlock; + usedpages += err; + /* chain the new scatterlist with previous one */ + if (cnt) + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]); + + iov_iter_advance(&msg->msg_iter, err); + cnt++; + } + + /* ensure output buffer is sufficiently large */ + if (usedpages < akcipher_calcsize(ctx)) { + err = -EMSGSIZE; + goto unlock; + } Why is the size of the output buffer enforced here instead of depending on the algorithm implementation? akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the algorithm generates as output during its operation. The code ensures that the caller provided at least that amount of memory for the kernel to store its data in. This check therefore is present to ensure the kernel does not overstep memory boundaries in user space. Yes, it's understood that the userspace buffer length must not be exceeded. But dst_len is part of the akcipher_request struct, so why does it need to be checked *here* when it is also checked later? 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. -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau: Hi Mat, > On Wed, 8 Jun 2016, Stephan Mueller wrote: > > Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau: > > > > Hi Mat, > > > >>> + used = ctx->used; > >>> + > >>> + /* convert iovecs of output buffers into scatterlists */ > >>> + while (iov_iter_count(&msg->msg_iter)) { > >>> + /* make one iovec available as scatterlist */ > >>> + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter, > >>> + iov_iter_count(&msg->msg_iter)); > >>> + if (err < 0) > >>> + goto unlock; > >>> + usedpages += err; > >>> + /* chain the new scatterlist with previous one */ > >>> + if (cnt) > >>> + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]); > >>> + > >>> + iov_iter_advance(&msg->msg_iter, err); > >>> + cnt++; > >>> + } > >>> + > >>> + /* ensure output buffer is sufficiently large */ > >>> + if (usedpages < akcipher_calcsize(ctx)) { > >>> + err = -EMSGSIZE; > >>> + goto unlock; > >>> + } > >> > >> Why is the size of the output buffer enforced here instead of depending > >> on > >> the algorithm implementation? > > > > akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size > > the > > algorithm generates as output during its operation. > > > > The code ensures that the caller provided at least that amount of memory > > for the kernel to store its data in. This check therefore is present to > > ensure the kernel does not overstep memory boundaries in user space. > > Yes, it's understood that the userspace buffer length must not be > exceeded. But dst_len is part of the akcipher_request struct, so why does > it need to be checked *here* when it is also checked later? I am always uneasy when the kernel has a user space interface and expects layers deep down inside the kernel to check for user space related boundaries. Note, we do not hand the __user flag down, so sparse and other tools cannot detect whether a particular cipher implementation has the right checks. I therefore always would like to check parameters at the interface handling logic. Cryptographers rightly should worry about their code implementing the cipher correctly. But I do not think that the cipher implementations should worry about security implications since they may be called from user space. > > > 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. Is your concern that we have a double check check for lengths here? If yes, I think we can live with an additional if() here. Or is your concern that the user space interface restricts things too much and thus prevents a valid use case? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
On Thu, 9 Jun 2016, Stephan Mueller wrote: Am Mittwoch, 8. Juni 2016, 12:14:49 schrieb Mat Martineau: Hi Mat, On Wed, 8 Jun 2016, Stephan Mueller wrote: Am Dienstag, 7. Juni 2016, 17:28:07 schrieb Mat Martineau: Hi Mat, + used = ctx->used; + + /* convert iovecs of output buffers into scatterlists */ + while (iov_iter_count(&msg->msg_iter)) { + /* make one iovec available as scatterlist */ + err = af_alg_make_sg(&ctx->rsgl[cnt], &msg->msg_iter, +iov_iter_count(&msg->msg_iter)); + if (err < 0) + goto unlock; + usedpages += err; + /* chain the new scatterlist with previous one */ + if (cnt) + af_alg_link_sg(&ctx->rsgl[cnt - 1], &ctx->rsgl[cnt]); + + iov_iter_advance(&msg->msg_iter, err); + cnt++; + } + + /* ensure output buffer is sufficiently large */ + if (usedpages < akcipher_calcsize(ctx)) { + err = -EMSGSIZE; + goto unlock; + } Why is the size of the output buffer enforced here instead of depending on the algorithm implementation? akcipher_calcsize calls crypto_akcipher_maxsize to get the maximum size the algorithm generates as output during its operation. The code ensures that the caller provided at least that amount of memory for the kernel to store its data in. This check therefore is present to ensure the kernel does not overstep memory boundaries in user space. Yes, it's understood that the userspace buffer length must not be exceeded. But dst_len is part of the akcipher_request struct, so why does it need to be checked *here* when it is also checked later? I am always uneasy when the kernel has a user space interface and expects layers deep down inside the kernel to check for user space related boundaries. Note, we do not hand the __user flag down, so sparse and other tools cannot detect whether a particular cipher implementation has the right checks. I therefore always would like to check parameters at the interface handling logic. Cryptographers rightly should worry about their code implementing the cipher correctly. But I do not think that the cipher implementations should worry about security implications since they may be called from user space. Userspace or not, buffer lengths need to be strictly checked. 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. Is your concern that we have a double check check for lengths here? If yes, I think we can live with an additional if() here. Or is your concern that the user space interface restricts things too much and thus prevents a valid use case? The latter - my primary concern is the constraint this places on userspace by forcing larger buffer sizes than might be necessary for the operation. struct akcipher_request has separate members for src_len and dst_len, and dst_len is documented as needing "to be at least as big as the expected result depending on the operation". Not the maximum result, the expected result. It's also documented that the cipher will generate an error if dst_len is insufficient and update the value with the required size. I'm updating some userspace TLS code that worked with an earlier, unmerged patch set for AF_ALG akcipher (from last year). The read calls with shorter buffers were the main porting problem. -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau: Hi Mat, > > Or is your concern that the user space interface restricts things too much > > and thus prevents a valid use case? > > The latter - my primary concern is the constraint this places on userspace > by forcing larger buffer sizes than might be necessary for the operation. > struct akcipher_request has separate members for src_len and dst_len, and > dst_len is documented as needing "to be at least as big as the expected > result depending on the operation". Not the maximum result, the expected > result. It's also documented that the cipher will generate an error if > dst_len is insufficient and update the value with the required size. > > I'm updating some userspace TLS code that worked with an earlier, unmerged > patch set for AF_ALG akcipher (from last year). The read calls with > shorter buffers were the main porting problem. I see -- are you proposing to drop that check entirely? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
On Thu, 9 Jun 2016, Stephan Mueller wrote: Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau: Hi Mat, Or is your concern that the user space interface restricts things too much and thus prevents a valid use case? The latter - my primary concern is the constraint this places on userspace by forcing larger buffer sizes than might be necessary for the operation. struct akcipher_request has separate members for src_len and dst_len, and dst_len is documented as needing "to be at least as big as the expected result depending on the operation". Not the maximum result, the expected result. It's also documented that the cipher will generate an error if dst_len is insufficient and update the value with the required size. I'm updating some userspace TLS code that worked with an earlier, unmerged patch set for AF_ALG akcipher (from last year). The read calls with shorter buffers were the main porting problem. I see -- are you proposing to drop that check entirely? Yes. Best regards, -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau: Hi Mat, Tadeusz, > On Thu, 9 Jun 2016, Stephan Mueller wrote: > > Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau: > > > > Hi Mat, > > > >>> Or is your concern that the user space interface restricts things too > >>> much > >>> and thus prevents a valid use case? > >> > >> The latter - my primary concern is the constraint this places on > >> userspace > >> by forcing larger buffer sizes than might be necessary for the operation. > >> struct akcipher_request has separate members for src_len and dst_len, and > >> dst_len is documented as needing "to be at least as big as the expected > >> result depending on the operation". Not the maximum result, the expected > >> result. It's also documented that the cipher will generate an error if > >> dst_len is insufficient and update the value with the required size. > >> > >> I'm updating some userspace TLS code that worked with an earlier, > >> unmerged > >> patch set for AF_ALG akcipher (from last year). The read calls with > >> shorter buffers were the main porting problem. > > > > I see -- are you proposing to drop that check entirely? > > Yes. Ok, after checking the code again, I think that dropping that sanity check should be ok given that this length is part of the akcipher API. Tadeusz, as you are currently managing that patch set, would you re-spin it with the following check removed? + if (usedpages < akcipher_calcsize(ctx)) { + err = -EMSGSIZE; + goto unlock; + } Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
On 06/09/2016 11:36 AM, Stephan Mueller wrote: > Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau: > > Hi Mat, Tadeusz, > >> On Thu, 9 Jun 2016, Stephan Mueller wrote: >>> Am Donnerstag, 9. Juni 2016, 11:18:04 schrieb Mat Martineau: >>> >>> Hi Mat, >>> > Or is your concern that the user space interface restricts things too > much > and thus prevents a valid use case? The latter - my primary concern is the constraint this places on userspace by forcing larger buffer sizes than might be necessary for the operation. struct akcipher_request has separate members for src_len and dst_len, and dst_len is documented as needing "to be at least as big as the expected result depending on the operation". Not the maximum result, the expected result. It's also documented that the cipher will generate an error if dst_len is insufficient and update the value with the required size. I'm updating some userspace TLS code that worked with an earlier, unmerged patch set for AF_ALG akcipher (from last year). The read calls with shorter buffers were the main porting problem. >>> >>> I see -- are you proposing to drop that check entirely? >> >> Yes. > > Ok, after checking the code again, I think that dropping that sanity check > should be ok given that this length is part of the akcipher API. > > Tadeusz, as you are currently managing that patch set, would you re-spin it > with the following check removed? > > + if (usedpages < akcipher_calcsize(ctx)) { > + err = -EMSGSIZE; > + goto unlock; > + } > Ok, I'll update the patch. Thanks, -- TS -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Hi, On 8 June 2016 at 21:14, Mat Martineau 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski: Hi Andrew, > Hi, > > On 8 June 2016 at 21:14, Mat Martineau > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Hi Stephan, On 14 June 2016 at 07:12, Stephan Mueller wrote: > Am Dienstag, 14. Juni 2016, 00:16:11 schrieb Andrew Zaborowski: >> On 8 June 2016 at 21:14, Mat Martineau >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Stephan, On Sat, 14 May 2016, Tadeusz Struk wrote: From: Stephan Mueller This patch adds the user space interface for asymmetric ciphers. The interface allows the use of sendmsg as well as vmsplice to provide data. This version has been rebased on top of 4.6 and a few chackpatch issues have been fixed. Signed-off-by: Stephan Mueller Signed-off-by: Tadeusz Struk --- crypto/algif_akcipher.c | 542 +++ 1 file changed, 542 insertions(+) create mode 100644 crypto/algif_akcipher.c diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c new file mode 100644 index 000..6342b6e --- /dev/null +++ b/crypto/algif_akcipher.c + +static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg, + size_t size) +{ + struct sock *sk = sock->sk; + struct alg_sock *ask = alg_sk(sk); + struct akcipher_ctx *ctx = ask->private; + struct akcipher_sg_list *sgl = &ctx->tsgl; + struct af_alg_control con = {}; + long copied = 0; + int op = 0; + bool init = 0; + int err; + + if (msg->msg_controllen) { + err = af_alg_cmsg_send(msg, &con); + if (err) + return err; + + init = 1; + switch (con.op) { + case ALG_OP_VERIFY: + case ALG_OP_SIGN: + case ALG_OP_ENCRYPT: + case ALG_OP_DECRYPT: + op = con.op; + break; + default: + return -EINVAL; + } + } + + lock_sock(sk); + if (!ctx->more && ctx->used) + goto unlock; err might be uninitialised at this goto. Should it be set to something like -EALREADY to indicate that data is already queued for a different crypto op? +unlock: + akcipher_data_wakeup(sk); + release_sock(sk); + + return err ?: copied; +} Regards, -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Dienstag, 14. Juni 2016, 10:22:15 schrieb Mat Martineau: Hi Mat, > Stephan, > > On Sat, 14 May 2016, Tadeusz Struk wrote: > > From: Stephan Mueller > > > > This patch adds the user space interface for asymmetric ciphers. The > > interface allows the use of sendmsg as well as vmsplice to provide data. > > > > This version has been rebased on top of 4.6 and a few chackpatch issues > > have been fixed. > > > > Signed-off-by: Stephan Mueller > > Signed-off-by: Tadeusz Struk > > --- > > crypto/algif_akcipher.c | 542 > > +++ 1 file changed, 542 > > insertions(+) > > create mode 100644 crypto/algif_akcipher.c > > > > diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c > > new file mode 100644 > > index 000..6342b6e > > --- /dev/null > > +++ b/crypto/algif_akcipher.c > > + > > +static int akcipher_sendmsg(struct socket *sock, struct msghdr *msg, > > + size_t size) > > +{ > > + struct sock *sk = sock->sk; > > + struct alg_sock *ask = alg_sk(sk); > > + struct akcipher_ctx *ctx = ask->private; > > + struct akcipher_sg_list *sgl = &ctx->tsgl; > > + struct af_alg_control con = {}; > > + long copied = 0; > > + int op = 0; > > + bool init = 0; > > + int err; > > + > > + if (msg->msg_controllen) { > > + err = af_alg_cmsg_send(msg, &con); > > + if (err) > > + return err; > > + > > + init = 1; > > + switch (con.op) { > > + case ALG_OP_VERIFY: > > + case ALG_OP_SIGN: > > + case ALG_OP_ENCRYPT: > > + case ALG_OP_DECRYPT: > > + op = con.op; > > + break; > > + default: > > + return -EINVAL; > > + } > > + } > > + > > + lock_sock(sk); > > + if (!ctx->more && ctx->used) > > + goto unlock; > > err might be uninitialised at this goto. Should it be set to something > like -EALREADY to indicate that data is already queued for a different > crypto op? Thanks for the hint. Tadeusz, I will provide you with an updated algif_akcipher.c for your patchset. I will also have a look at the comment from Andrew. > > > > > +unlock: > > + akcipher_data_wakeup(sk); > > + release_sock(sk); > > + > > + return err ?: copied; > > +} > > Regards, > > -- > Mat Martineau > Intel OTC Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Hi Stephan, On 16 June 2016 at 10:05, Stephan Mueller 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Donnerstag, 16. Juni 2016, 16:59:01 schrieb Andrew Zaborowski: Hi Andrew, > Hi Stephan, > > On 16 June 2016 at 10:05, Stephan Mueller 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Hi Stephan, On 16 June 2016 at 17:38, Stephan Mueller 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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Stephan and Tadeusz, On Fri, 10 Jun 2016, Tadeusz Struk wrote: On 06/09/2016 11:36 AM, Stephan Mueller wrote: Am Donnerstag, 9. Juni 2016, 11:27:13 schrieb Mat Martineau: Hi Mat, Tadeusz, Ok, after checking the code again, I think that dropping that sanity check should be ok given that this length is part of the akcipher API. Tadeusz, as you are currently managing that patch set, would you re-spin it with the following check removed? + if (usedpages < akcipher_calcsize(ctx)) { + err = -EMSGSIZE; + goto unlock; + } Ok, I'll update the patch. Thanks, that helps (especially with pkcs1pad). This brings me to another proposal for read buffer sizing: AF_ALG akcipher can guarantee that partial reads (where the read buffer is shorter than the output of the crypto op) will work using the same semantics as SOCK_DGRAM/SOCK_SEQPACKET. With those sockets, as much data as will fit is copied in to the read buffer and the remainder is discarded. I realize there's a performance and memory tradeoff, since the crypto algorithm needs a sufficiently large output buffer that would have to be created by AF_ALG akcipher. The user could manage that tradeoff by providing a larger buffer (typically key_size?) if it wants to avoid allocating and copying intermediate buffers inside the kernel. -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Am Mittwoch, 22. Juni 2016, 15:45:38 schrieb Mat Martineau: Hi Mat, > > > > Ok, I'll update the patch. > > Thanks, that helps (especially with pkcs1pad). Tadeusz received the updated patch from me to integrate it into his patch set. > > This brings me to another proposal for read buffer sizing: AF_ALG akcipher > can guarantee that partial reads (where the read buffer is shorter than > the output of the crypto op) will work using the same semantics as > SOCK_DGRAM/SOCK_SEQPACKET. With those sockets, as much data as will fit is > copied in to the read buffer and the remainder is discarded. > > I realize there's a performance and memory tradeoff, since the crypto > algorithm needs a sufficiently large output buffer that would have to be > created by AF_ALG akcipher. The user could manage that tradeoff by > providing a larger buffer (typically key_size?) if it wants to avoid > allocating and copying intermediate buffers inside the kernel. How shall the user know that something got truncated or that the kernel created memory? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Hi Stephan, >> This brings me to another proposal for read buffer sizing: AF_ALG akcipher can guarantee that partial reads (where the read buffer is shorter than the output of the crypto op) will work using the same semantics as SOCK_DGRAM/SOCK_SEQPACKET. With those sockets, as much data as will fit is copied in to the read buffer and the remainder is discarded. I realize there's a performance and memory tradeoff, since the crypto algorithm needs a sufficiently large output buffer that would have to be created by AF_ALG akcipher. The user could manage that tradeoff by providing a larger buffer (typically key_size?) if it wants to avoid allocating and copying intermediate buffers inside the kernel. How shall the user know that something got truncated or that the kernel created memory? To the former point, recall the signature of recv: ssize_t recv(int sockfd, void *buf, size_t len, int flags); Traditionally, userspace apps can know that the buffer provided to recv was too small in two ways: The return value from recv / recvmsg was >= len. In the case of recvmsg, the MSG_TRUNC flag is set. To quote man recv: "All three calls return the length of the message on successful compleā tion. If a message is too long to fit in the supplied buffer, excess bytes may be discarded depending on the type of socket the message is received from." and "MSG_TRUNC (since Linux 2.2) For raw (AF_PACKET), Internet datagram (sinceLinux 2.4.27/2.6.8), netlink (since Linux 2.6.22), and UNIX datagram (since Linux 3.4) sockets: return the real length of the packet or datagram, even when it was longer than the passed buffer. " Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html