crypto: af_alg - Remove zero-copy support from AF_ALG
Subsystems (3)
PATCH 1/1 crypto: af_alg - Remove zero-copy support from AF_ALG
The zero-copy support is one of the riskiest aspects of AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.
While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in AF_ALG, which is a niche UAPI. AF_ALG primarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.
Originally AF_ALG was intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.
Thus, the risks of the zero-copy support in AF_ALG vastly outweigh its benefits. Just remove it.
Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.
--- Documentation/crypto/userspace-if.rst | 30 ++--------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 32 insertions(+), 79 deletions(-)
Documentation/crypto/userspace-if.rst +3 -27
@@ -325,37 +325,13 @@ CRYPTO_USER_API_RNG_CAVP option: but only after the entropy has been set. Zero-Copy Interface ------------------- -In addition to the send/write/read/recv system call family, the AF_ALG -interface can be accessed with the zero-copy interface of -splice/vmsplice. As the name indicates, the kernel tries to avoid a copy -operation into kernel space. - -The zero-copy operation requires data to be aligned at the page -boundary. Non-aligned data can be used as well, but may require more -operations of the kernel which would defeat the speed gains obtained -from the zero-copy interface. - -The system-inherent limit for the size of one zero-copy operation is 16 -pages. If more data is to be sent to AF_ALG, user space must slice the -input into segments with a maximum size of 16 pages. - -Zero-copy can be used with the following code example (a complete -working example is provided with libkcapi): - -:: - - int pipes[2]; - - pipe(pipes); - /* input data in iov */ - vmsplice(pipes[1], iov, iovlen, SPLICE_F_GIFT); - /* opfd is the file descriptor returned from accept() system call */ - splice(pipes[0], NULL, opfd, NULL, ret, 0); - read(opfd, out, outlen); +AF_ALG used to have zero-copy support, but it was removed due to it being a +frequent source of vulnerabilities. For backwards compatibility the splice +system call is still supported, but the data will simply be copied. Setsockopt Interface --------------------
crypto/af_alg.c +25 -48
100 lines not on this page · raw message
crypto/algif_aead.c +4 -4
@@ -7,14 +7,14 @@* This file provides the user-space API for AEAD ciphers. * * The following concept of the memory management is used: * * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is -* filled by user space with the data submitted via sendmsg (maybe with -* MSG_SPLICE_PAGES). Filling up the TX SGL does not cause a crypto operation -* -- the data will only be tracked by the kernel. Upon receipt of one recvmsg -* call, the caller must provide a buffer which is tracked with the RX SGL. +* filled by user space with the data submitted via sendmsg. Filling up the TX +* SGL does not cause a crypto operation -- the data will only be tracked by the +* kernel. Upon receipt of one recvmsg call, the caller must provide a buffer +* which is tracked with the RX SGL. * * During the processing of the recvmsg operation, the cipher request is * allocated and prepared. As part of the recvmsg operation, the processed * TX buffers are extracted from the TX SGL into a separate SGL. *
base-commit: 6d35786de28116ecf78797a62b84e6bf3c45aa5a
Signature · 1 line
2.54.0
Quoted 213 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Reported-by Feng Ning <[email protected]>Signed-off-by Eric Biggers <[email protected]>--- Documentation/crypto/userspace-if.rst | 30 ++--------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 32 insertions(+), 79 deletions(-)Documentation/crypto/userspace-if.rst +3
-27@@ -325,37 +325,13 @@ CRYPTO_USER_API_RNG_CAVP option: but only after the entropy has been set. Zero-Copy Interface ------------------- -In addition to the send/write/read/recv system call family, the AF_ALG -interface can be accessed with the zero-copy interface of -splice/vmsplice. As the name indicates, the kernel tries to avoid a copy -operation into kernel space. - -The zero-copy operation requires data to be aligned at the page -boundary. Non-aligned data can be used as well, but may require more -operations of the kernel which would defeat the speed gains obtained -from the zero-copy interface. - -The system-inherent limit for the size of one zero-copy operation is 16 -pages. If more data is to be sent to AF_ALG, user space must slice the -input into segments with a maximum size of 16 pages. - -Zero-copy can be used with the following code example (a complete -working example is provided with libkcapi): - -:: - - int pipes[2]; - - pipe(pipes); - /* input data in iov */ - vmsplice(pipes[1], iov, iovlen, SPLICE_F_GIFT); - /* opfd is the file descriptor returned from accept() system call */ - splice(pipes[0], NULL, opfd, NULL, ret, 0); - read(opfd, out, outlen); +AF_ALG used to have zero-copy support, but it was removed due to it being a +frequent source of vulnerabilities. For backwards compatibility the splice +system call is still supported, but the data will simply be copied. Setsockopt Interface --------------------crypto/af_alg.c +25
-48@@ -971,11 +971,11 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,struct scatterlist *sg;size_t len = size;ssize_t plen;/* use the existing memory in an allocated page */ -if (ctx->merge && !(msg->msg_flags &MSG_SPLICE_PAGES )) { +if (ctx->merge) { sgl =list_entry (ctx->tsgl_list.prev, struct af_alg_tsgl, list); sg = sgl->sg + sgl->cur -1 ; len =min_t (size_t, len,PAGE_SIZE - sg->offset - sg->length);@@ -1015,64 +1015,41 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size, list); sg = sgl->sg;if (sgl->cur)sg_unmark_end (sg + sgl->cur -1 ); -if (msg->msg_flags &MSG_SPLICE_PAGES ) { -struct sg_table sgtable = { - .sgl = sg, - .nents = sgl->cur, - .orig_nents = sgl->cur, - }; - - plen =extract_iter_to_sg (&msg->msg_iter, len, &sgtable, -MAX_SGL_ENTS - sgl->cur,0 ); -if (plen <0 ) { - err = plen; +do { +struct page *pg; +unsigned int i = sgl->cur; + + plen =min_t (size_t, len,PAGE_SIZE ); + + pg =alloc_page (GFP_KERNEL ); +if (!pg) { + err = -ENOMEM ; goto unlock; } -for (; sgl->cur < sgtable.nents; sgl->cur++) -get_page (sg_page (&sg[sgl->cur])); +sg_assign_page (sg + i, pg); + + err =memcpy_from_msg (page_address (sg_page (sg + i)), + msg, plen); +if (err) { +__free_page (sg_page (sg + i)); +sg_assign_page (sg + i,NULL ); + goto unlock; + } + + sg[i].length = plen; len -= plen; ctx->used += plen; copied += plen; size -= plen; - }else { -do { -struct page *pg; -unsigned int i = sgl->cur; - - plen =min_t (size_t, len,PAGE_SIZE ); - - pg =alloc_page (GFP_KERNEL ); -if (!pg) { - err = -ENOMEM ; - goto unlock; - } - -sg_assign_page (sg + i, pg); - - err =memcpy_from_msg ( -page_address (sg_page (sg + i)), - msg, plen); -if (err) { -__free_page (sg_page (sg + i)); -sg_assign_page (sg + i,NULL ); - goto unlock; - } - - sg[i].length = plen; - len -= plen; - ctx->used += plen; - copied += plen; - size -= plen; - sgl->cur++; - }while (len && sgl->cur <MAX_SGL_ENTS ); - - ctx->merge = plen & (PAGE_SIZE -1 ); - } + sgl->cur++; + }while (len && sgl->cur <MAX_SGL_ENTS ); + + ctx->merge = plen & (PAGE_SIZE -1 );if (!size)sg_mark_end (sg + sgl->cur -1 ); }crypto/algif_aead.c +4
-4@@ -7,14 +7,14 @@* This file provides the user-space API for AEAD ciphers. * * The following concept of the memory management is used: * * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is -* filled by user space with the data submitted via sendmsg (maybe with -* MSG_SPLICE_PAGES). Filling up the TX SGL does not cause a crypto operation -* -- the data will only be tracked by the kernel. Upon receipt of one recvmsg -* call, the caller must provide a buffer which is tracked with the RX SGL. +* filled by user space with the data submitted via sendmsg. Filling up the TX +* SGL does not cause a crypto operation -- the data will only be tracked by the +* kernel. Upon receipt of one recvmsg call, the caller must provide a buffer +* which is tracked with the RX SGL. * * During the processing of the recvmsg operation, the cipher request is * allocated and prepared. As part of the recvmsg operation, the processed * TX buffers are extracted from the TX SGL into a separate SGL. *
Quoted 1 line
base-commit:
6d35786de28116ecf78797a62b84e6bf3c45aa5a
In light of https://lore.kernel.org/all/[email protected]/,
yes please!
Should there be a Link: tag referencing that email?
With or without it:
Signature · 2 lines
Sincerely, Demi Marie Obenour (she/her/hers)
Quoted 31 lines · Demi Marie Obenour
Quoted 30 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Reported-by Feng Ning <[email protected]>
[...]
Quoted 4 lines
In light of https://lore.kernel.org/all/[email protected]/,
yes please!Should there be a Link: tag referencing that email?
Yes I forgot to put that in, sorry. It should go after the second Reported-by:
- Eric
Quoted 36 lines · Eric Biggers
Quoted 31 lines · Demi Marie Obenour
Quoted 30 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Reported-by Feng Ning <[email protected]>[...]
Quoted 3 lines
In light of https://lore.kernel.org/all/[email protected]/,
yes please!
Quoted 8 lines
Quoted 1 line
Should there be a Link: tag referencing that email?
Yes I forgot to put that in, sorry. It should go after the second Reported-by:
- Eric
Should this also link to copy.fail or a related email?
Signature · 2 lines
Sincerely, Demi Marie Obenour (she/her/hers)
Quoted 39 lines · Demi Marie Obenour
Quoted 38 lines · Eric Biggers
Quoted 31 lines · Demi Marie Obenour
Quoted 30 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Reported-by Feng Ning <[email protected]>[...]
Quoted 4 lines
In light of https://lore.kernel.org/all/[email protected]/,
yes please!Should there be a Link: tag referencing that email?
Quoted 8 lines
Quoted 6 lines
Yes I forgot to put that in, sorry. It should go after the second Reported-by:
- Eric
Should this also link to copy.fail or a related email?
Sure. I'll go ahead and resend right away, so it doesn't get applied without the links.
- Eric
PATCH v2 1/1 crypto: af_alg - Remove zero-copy support from AF_ALG
The zero-copy support is one of the riskiest aspects of AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.
While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in AF_ALG, which is a niche UAPI. AF_ALG primarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.
Originally AF_ALG was intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.
Thus, the risks of the zero-copy support in AF_ALG vastly outweigh its benefits. Just remove it.
Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Tested with libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.
---
v2: added tags
Documentation/crypto/userspace-if.rst | 30 ++--------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 32 insertions(+), 79 deletions(-)
Documentation/crypto/userspace-if.rst +3 -27
@@ -325,37 +325,13 @@ CRYPTO_USER_API_RNG_CAVP option: but only after the entropy has been set. Zero-Copy Interface ------------------- -In addition to the send/write/read/recv system call family, the AF_ALG -interface can be accessed with the zero-copy interface of -splice/vmsplice. As the name indicates, the kernel tries to avoid a copy -operation into kernel space. - -The zero-copy operation requires data to be aligned at the page -boundary. Non-aligned data can be used as well, but may require more -operations of the kernel which would defeat the speed gains obtained -from the zero-copy interface. - -The system-inherent limit for the size of one zero-copy operation is 16 -pages. If more data is to be sent to AF_ALG, user space must slice the -input into segments with a maximum size of 16 pages. - -Zero-copy can be used with the following code example (a complete -working example is provided with libkcapi): - -:: - - int pipes[2]; - - pipe(pipes); - /* input data in iov */ - vmsplice(pipes[1], iov, iovlen, SPLICE_F_GIFT); - /* opfd is the file descriptor returned from accept() system call */ - splice(pipes[0], NULL, opfd, NULL, ret, 0); - read(opfd, out, outlen); +AF_ALG used to have zero-copy support, but it was removed due to it being a +frequent source of vulnerabilities. For backwards compatibility the splice +system call is still supported, but the data will simply be copied. Setsockopt Interface --------------------
crypto/af_alg.c +25 -48
100 lines not on this page · raw message
crypto/algif_aead.c +4 -4
@@ -7,14 +7,14 @@* This file provides the user-space API for AEAD ciphers. * * The following concept of the memory management is used: * * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is -* filled by user space with the data submitted via sendmsg (maybe with -* MSG_SPLICE_PAGES). Filling up the TX SGL does not cause a crypto operation -* -- the data will only be tracked by the kernel. Upon receipt of one recvmsg -* call, the caller must provide a buffer which is tracked with the RX SGL. +* filled by user space with the data submitted via sendmsg. Filling up the TX +* SGL does not cause a crypto operation -- the data will only be tracked by the +* kernel. Upon receipt of one recvmsg call, the caller must provide a buffer +* which is tracked with the RX SGL. * * During the processing of the recvmsg operation, the cipher request is * allocated and prepared. As part of the recvmsg operation, the processed * TX buffers are extracted from the TX SGL into a separate SGL. *
base-commit: 6d35786de28116ecf78797a62b84e6bf3c45aa5a
Signature · 1 line
2.54.0
Quoted 2 lines · Eric Biggers
Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.
I realized that the "hash" algorithm type still does zero-copy even after this patch. I.e. this patch affects only "skcipher" and "aead". Those are the more important ones, but we should do "hash" too.
The diff is still fine, but I'll adjust the patch description to clarify the scope, and send a separate patch to handle "hash".
- Eric
PATCH v3 1/1 crypto: af_alg - Remove zero-copy support from skcipher and aead
The zero-copy support is one of the riskiest aspects of AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a recipe for TOCTOU vulnerabilities.
While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in AF_ALG, which is a niche UAPI. AF_ALG primarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.
Originally AF_ALG was intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.
Thus, the risks of the zero-copy support in AF_ALG vastly outweigh its benefits. Let's just remove it.
This commit removes it from the "skcipher" and "aead" algorithm types. "hash" will be handled separately.
This is a soft break, not a hard break. Even after this commit, it still works to use splice() or sendfile() to transfer data to an AF_ALG request socket from a pipe or any file, respectively. What changes is just that the kernel now makes an internal, stable copy of the data before doing the crypto operation. So performance is slightly reduced, but the UAPI isn't broken. And, very importantly, it's much safer.
Tested with libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well. I also used a custom test program to verify that sendfile() still works.
---
v3: improved explanation
v2: added tags
Documentation/crypto/userspace-if.rst | 31 ++---------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 33 insertions(+), 79 deletions(-)
Documentation/crypto/userspace-if.rst +4 -27
@@ -325,37 +325,14 @@ CRYPTO_USER_API_RNG_CAVP option: but only after the entropy has been set. Zero-Copy Interface ------------------- -In addition to the send/write/read/recv system call family, the AF_ALG -interface can be accessed with the zero-copy interface of -splice/vmsplice. As the name indicates, the kernel tries to avoid a copy -operation into kernel space. - -The zero-copy operation requires data to be aligned at the page -boundary. Non-aligned data can be used as well, but may require more -operations of the kernel which would defeat the speed gains obtained -from the zero-copy interface. - -The system-inherent limit for the size of one zero-copy operation is 16 -pages. If more data is to be sent to AF_ALG, user space must slice the -input into segments with a maximum size of 16 pages. - -Zero-copy can be used with the following code example (a complete -working example is provided with libkcapi): - -:: - - int pipes[2]; - - pipe(pipes); - /* input data in iov */ - vmsplice(pipes[1], iov, iovlen, SPLICE_F_GIFT); - /* opfd is the file descriptor returned from accept() system call */ - splice(pipes[0], NULL, opfd, NULL, ret, 0); - read(opfd, out, outlen); +AF_ALG used to have zero-copy support, but it was removed due to it being a +frequent source of vulnerabilities. For backwards compatibility the splice() +and sendfile() system calls are still supported, but the kernel will make an +internal copy of the data before passing it to the crypto code. Setsockopt Interface --------------------
crypto/af_alg.c +25 -48
100 lines not on this page · raw message
crypto/algif_aead.c +4 -4
@@ -7,14 +7,14 @@* This file provides the user-space API for AEAD ciphers. * * The following concept of the memory management is used: * * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is -* filled by user space with the data submitted via sendmsg (maybe with -* MSG_SPLICE_PAGES). Filling up the TX SGL does not cause a crypto operation -* -- the data will only be tracked by the kernel. Upon receipt of one recvmsg -* call, the caller must provide a buffer which is tracked with the RX SGL. +* filled by user space with the data submitted via sendmsg. Filling up the TX +* SGL does not cause a crypto operation -- the data will only be tracked by the +* kernel. Upon receipt of one recvmsg call, the caller must provide a buffer +* which is tracked with the RX SGL. * * During the processing of the recvmsg operation, the cipher request is * allocated and prepared. As part of the recvmsg operation, the processed * TX buffers are extracted from the TX SGL into a separate SGL. *
base-commit: c7e4e4d5f7dc2daa439303d1b5bf6bdfaa249f49
Signature · 1 line
2.54.0
Herbert Xu
maintainerRe: [PATCH v3] crypto: af_alg - Remove zero-copy support from skcipher and aead
Quoted 44 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Let's just remove it.This commit removes it from the "skcipher" and "aead" algorithm types. "hash" will be handled separately.
This is a soft break, not a hard break. Even after this commit, it still works to use
splice()orsendfile()to transfer data to anAF_ALGrequest socket from a pipe or any file, respectively. What changes is just that the kernel now makes an internal, stable copy of the data before doing the crypto operation. So performance is slightly reduced, but the UAPI isn't broken. And, very importantly, it's much safer.Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well. I also used a custom test program to verify thatsendfile()still works.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Link https://copy.fail/Reported-by Feng Ning <[email protected]>Reviewed-by Demi Marie Obenour <[email protected]>Signed-off-by Eric Biggers <[email protected]>---
Quoted 7 lines
v3: improved explanation
v2: added tagsDocumentation/crypto/userspace-if.rst | 31 ++---------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 33 insertions(+), 79 deletions(-)
Patch applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 16 lines · Eric Biggers
The zero-copy support is one of the riskiest aspects of
AF_ALG. It allows userspace to request cryptographic operations directly on pagecache pages of files like the 'su' binary. It also allows userspace to concurrently modify the memory which is being operated on, a huge recipe for TOCTOU vulnerabilities.While zero-copy support is more valuable in other areas of the kernel like the frequently used networking and file I/O code, it has far less value in
AF_ALG, which is a niche UAPI.AF_ALGprimarily just exists for backwards compatibility with a small set of userspace programs such as 'iwd' that haven't yet been fixed to use userspace crypto code.Originally
AF_ALGwas intended to be used to access hardware crypto accelerators. However, it isn't an efficient interface for that anyway, and it turned out to be rarely used in this way in practice.
Quoted 6 lines
Thus, the risks of the zero-copy support in
AF_ALGvastly outweigh its benefits. Just remove it.Note that this isn't a hard break, since the splice syscall is still supported. The data is just now copied instead. So it still works, just a bit slower in some cases.
Syd sandbox uses AF_ALG zero-copy for its Force Sandboxing[1] and Crypt Sandboxing[1]. Zero-copy means Syd does not have to copy sandbox process data into its own address space providing safety and security. Switching to read/write rather than pipes and splice breaks a fundamental safety guarantee for the sandbox. Please do not break userspace.
Will sendfile(2) continue to work? How can i test? Please help me.
[1]: https://man.exherbo.org/syd.7.html#Force_Sandboxing
[2]: https://man.exherbo.org/syd.7.html#Crypt_Sandboxing
Quoted 196 lines
Tested with
libkcapi/test.sh. All its test cases still pass. I also verified that this would have prevented the copy.fail exploit as well.Fixes 8ff590903d5f ("crypto: algif_skcipher - User-space interface for skcipher operations")Fixes 400c40cf78da ("crypto: algif - add AEAD support")Reported-by Taeyang Lee <[email protected]>Reported-by Feng Ning <[email protected]>Signed-off-by Eric Biggers <[email protected]>--- Documentation/crypto/userspace-if.rst | 30 ++--------- crypto/af_alg.c | 73 +++++++++------------------ crypto/algif_aead.c | 8 +-- 3 files changed, 32 insertions(+), 79 deletions(-)Documentation/crypto/userspace-if.rst +3
-27@@ -325,37 +325,13 @@ CRYPTO_USER_API_RNG_CAVP option: but only after the entropy has been set. Zero-Copy Interface ------------------- -In addition to the send/write/read/recv system call family, the AF_ALG -interface can be accessed with the zero-copy interface of -splice/vmsplice. As the name indicates, the kernel tries to avoid a copy -operation into kernel space. - -The zero-copy operation requires data to be aligned at the page -boundary. Non-aligned data can be used as well, but may require more -operations of the kernel which would defeat the speed gains obtained -from the zero-copy interface. - -The system-inherent limit for the size of one zero-copy operation is 16 -pages. If more data is to be sent to AF_ALG, user space must slice the -input into segments with a maximum size of 16 pages. - -Zero-copy can be used with the following code example (a complete -working example is provided with libkcapi): - -:: - - int pipes[2]; - - pipe(pipes); - /* input data in iov */ - vmsplice(pipes[1], iov, iovlen, SPLICE_F_GIFT); - /* opfd is the file descriptor returned from accept() system call */ - splice(pipes[0], NULL, opfd, NULL, ret, 0); - read(opfd, out, outlen); +AF_ALG used to have zero-copy support, but it was removed due to it being a +frequent source of vulnerabilities. For backwards compatibility the splice +system call is still supported, but the data will simply be copied. Setsockopt Interface --------------------crypto/af_alg.c +25
-48@@ -971,11 +971,11 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,struct scatterlist *sg;size_t len = size;ssize_t plen;/* use the existing memory in an allocated page */ -if (ctx->merge && !(msg->msg_flags &MSG_SPLICE_PAGES )) { +if (ctx->merge) { sgl =list_entry (ctx->tsgl_list.prev, struct af_alg_tsgl, list); sg = sgl->sg + sgl->cur -1 ; len =min_t (size_t, len,PAGE_SIZE - sg->offset - sg->length);@@ -1015,64 +1015,41 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size, list); sg = sgl->sg;if (sgl->cur)sg_unmark_end (sg + sgl->cur -1 ); -if (msg->msg_flags &MSG_SPLICE_PAGES ) { -struct sg_table sgtable = { - .sgl = sg, - .nents = sgl->cur, - .orig_nents = sgl->cur, - }; - - plen =extract_iter_to_sg (&msg->msg_iter, len, &sgtable, -MAX_SGL_ENTS - sgl->cur,0 ); -if (plen <0 ) { - err = plen; +do { +struct page *pg; +unsigned int i = sgl->cur; + + plen =min_t (size_t, len,PAGE_SIZE ); + + pg =alloc_page (GFP_KERNEL ); +if (!pg) { + err = -ENOMEM ; goto unlock; } -for (; sgl->cur < sgtable.nents; sgl->cur++) -get_page (sg_page (&sg[sgl->cur])); +sg_assign_page (sg + i, pg); + + err =memcpy_from_msg (page_address (sg_page (sg + i)), + msg, plen); +if (err) { +__free_page (sg_page (sg + i)); +sg_assign_page (sg + i,NULL ); + goto unlock; + } + + sg[i].length = plen; len -= plen; ctx->used += plen; copied += plen; size -= plen; - }else { -do { -struct page *pg; -unsigned int i = sgl->cur; - - plen =min_t (size_t, len,PAGE_SIZE ); - - pg =alloc_page (GFP_KERNEL ); -if (!pg) { - err = -ENOMEM ; - goto unlock; - } - -sg_assign_page (sg + i, pg); - - err =memcpy_from_msg ( -page_address (sg_page (sg + i)), - msg, plen); -if (err) { -__free_page (sg_page (sg + i)); -sg_assign_page (sg + i,NULL ); - goto unlock; - } - - sg[i].length = plen; - len -= plen; - ctx->used += plen; - copied += plen; - size -= plen; - sgl->cur++; - }while (len && sgl->cur <MAX_SGL_ENTS ); - - ctx->merge = plen & (PAGE_SIZE -1 ); - } + sgl->cur++; + }while (len && sgl->cur <MAX_SGL_ENTS ); + + ctx->merge = plen & (PAGE_SIZE -1 );if (!size)sg_mark_end (sg + sgl->cur -1 ); }crypto/algif_aead.c +4
-4@@ -7,14 +7,14 @@* This file provides the user-space API for AEAD ciphers. * * The following concept of the memory management is used: * * The kernel maintains two SGLs, the TX SGL and the RX SGL. The TX SGL is -* filled by user space with the data submitted via sendmsg (maybe with -* MSG_SPLICE_PAGES). Filling up the TX SGL does not cause a crypto operation -* -- the data will only be tracked by the kernel. Upon receipt of one recvmsg -* call, the caller must provide a buffer which is tracked with the RX SGL. +* filled by user space with the data submitted via sendmsg. Filling up the TX +* SGL does not cause a crypto operation -- the data will only be tracked by the +* kernel. Upon receipt of one recvmsg call, the caller must provide a buffer +* which is tracked with the RX SGL. * * During the processing of the recvmsg operation, the cipher request is * allocated and prepared. As part of the recvmsg operation, the processed * TX buffers are extracted from the TX SGL into a separate SGL. * base-commit:
6d35786de28116ecf78797a62b84e6bf3c45aa5a--
2.54.0
Quoted 10 lines · Ⓐlï P☮latel
Syd sandbox uses
AF_ALGzero-copy for its Force Sandboxing[1] and Crypt Sandboxing[1]. Zero-copy means Syd does not have to copy sandbox process data into its own address space providing safety and security. Switching to read/write rather than pipes and splice breaks a fundamental safety guarantee for the sandbox. Please do not break userspace.Will sendfile(2) continue to work?
[1]: https://man.exherbo.org/syd.7.html#Force_Sandboxing
[2]: https://man.exherbo.org/syd.7.html#Crypt_Sandboxing
It's very unclear what that feature (which I don't think anyone knew even existed) is trying to accomplish. Regardless, this patch doesn't break the splice or sendfile syscalls. It just makes them run a bit more slowly since the kernel will copy the data internally. So I think your concern isn't justified.
Quoted 1 line
How can i test? Please help me.
If this is a feature you care about, perhaps you know how to test it?
- Eric
Quoted 20 lines · Eric Biggers
Quoted 10 lines · Ⓐlï P☮latel
Syd sandbox uses
AF_ALGzero-copy for its Force Sandboxing[1] and Crypt Sandboxing[1]. Zero-copy means Syd does not have to copy sandbox process data into its own address space providing safety and security. Switching to read/write rather than pipes and splice breaks a fundamental safety guarantee for the sandbox. Please do not break userspace.Will sendfile(2) continue to work?
[1]: https://man.exherbo.org/syd.7.html#Force_Sandboxing
[2]: https://man.exherbo.org/syd.7.html#Crypt_SandboxingIt's very unclear what that feature (which I don't think anyone knew even existed) is trying to accomplish. Regardless, this patch doesn't break the splice or sendfile syscalls. It just makes them run a bit more slowly since the kernel will copy the data internally. So I think your concern isn't justified.
Quoted 4 lines
Quoted 1 line
How can i test? Please help me.
If this is a feature you care about, perhaps you know how to test it?
Thank you very much for the explanation and excuse me I panicked.
Quoted 2 lines
- Eric
Best,
Ali
Quoted 22 lines · Ⓐlï P☮latel
Quoted 19 lines · Eric Biggers
Quoted 10 lines · Ⓐlï P☮latel
Syd sandbox uses
AF_ALGzero-copy for its Force Sandboxing[1] and Crypt Sandboxing[1]. Zero-copy means Syd does not have to copy sandbox process data into its own address space providing safety and security. Switching to read/write rather than pipes and splice breaks a fundamental safety guarantee for the sandbox. Please do not break userspace.Will sendfile(2) continue to work?
[1]: https://man.exherbo.org/syd.7.html#Force_Sandboxing
[2]: https://man.exherbo.org/syd.7.html#Crypt_SandboxingIt's very unclear what that feature (which I don't think anyone knew even existed) is trying to accomplish. Regardless, this patch doesn't break the splice or sendfile syscalls. It just makes them run a bit more slowly since the kernel will copy the data internally. So I think your concern isn't justified.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
How can i test? Please help me.
If this is a feature you care about, perhaps you know how to test it?
Thank you very much for the explanation and excuse me I panicked.
Quoted 1 line
- Eric
I've tested that all three cases of read/write, sendfile, and vmsplice+splice still work. The difference is just in how the kernel implements them internally. See the following test program.
#define _GNU_SOURCE
#include <assert.h>
#include <fcntl.h>
#include <linux/if_alg.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <sys/sendfile.h>
#include <sys/socket.h>
#include <unistd.h>
int main(void)
{
for (int test = 0; test < 3; test++) {
uint8_t msg[32] = {};
uint8_t key[16] = {1,2,3,4};
struct sockaddr_alg addr = {
.salg_family = AF_ALG,
.salg_type = "skcipher",
.salg_name = "cbc(aes)",
};
int filefd, algfd, reqfd, pipefd[2], ret;
filefd = open("msg_file", O_RDWR|O_CREAT|O_TRUNC, 0600);
assert(filefd >= 0);
ret = pwrite(filefd, msg, sizeof(msg), 0);
assert(ret == sizeof(msg));
algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
assert(algfd >= 0);
ret = bind(algfd, (struct sockaddr *)&addr, sizeof(addr));
assert(ret == 0);
ret = setsockopt(algfd, SOL_ALG, ALG_SET_KEY, key, sizeof(key));
assert(ret == 0);
reqfd = accept(algfd, NULL, NULL);
assert(reqfd >= 0);
switch (test) {
case 0:
printf("read/write test\n");
ret = read(filefd, msg, sizeof(msg));
assert(ret == sizeof(msg));
ret = write(reqfd, msg, sizeof(msg));
assert(ret == sizeof(msg));
break;
case 1:
printf("sendfile test\n");
ret = sendfile(reqfd, filefd, NULL, sizeof(msg));
assert(ret == sizeof(msg));
break;
case 2:
printf("splice test\n");
ret = pipe(pipefd);
assert(ret == 0);
struct iovec iov = { .iov_base = msg, .iov_len = sizeof(msg) };
ret = vmsplice(pipefd[1], &iov, 1, SPLICE_F_GIFT);
assert(ret == sizeof(msg));
ret = splice(pipefd[0], NULL, reqfd, NULL, sizeof(msg), SPLICE_F_MOVE);
assert(ret == sizeof(msg));
break;
}
ret = read(reqfd, msg, sizeof(msg));
assert(ret == sizeof(msg));
for (int i = 0; i < sizeof(msg); i++)
printf("%02x ", msg[i]);
printf("\n");
}
}