Re: [PATCH net-next v2 5/6] mptcp: support rstreason for passive reset
On Thu, 4 Apr 2024, Jason Xing wrote:
From: Jason Xing
It relys on what reset options in MPTCP does as rfc8684 says. Reusing
this logic can save us much energy. This patch replaces all the prior
NOT_SPECIFIED reasons.
Signed-off-by: Jason Xing
---
net/mptcp/subflow.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index a68d5d0f3e2a..24668d3020aa 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -304,7 +304,10 @@ static struct dst_entry *subflow_v4_route_req(const struct
sock *sk,
dst_release(dst);
if (!req->syncookie)
- tcp_request_sock_ops.send_reset(sk, skb,
SK_RST_REASON_NOT_SPECIFIED);
+ /* According to RFC 8684, 3.2. Starting a New Subflow,
+* we should use an "MPTCP specific error" reason code.
+*/
+ tcp_request_sock_ops.send_reset(sk, skb,
SK_RST_REASON_MPTCP_RST_EMPTCP);
Hi Jason -
In this case, the MPTCP reset reason is set in subflow_check_req(). Looks
like it uses EMPTCP but that isn't guaranteed to stay the same. I think it
would be better to extract the reset reason from the skb extension or the
subflow context "reset_reason" field.
return NULL;
}
@@ -371,7 +374,10 @@ static struct dst_entry *subflow_v6_route_req(const struct
sock *sk,
dst_release(dst);
if (!req->syncookie)
- tcp6_request_sock_ops.send_reset(sk, skb,
SK_RST_REASON_NOT_SPECIFIED);
+ /* According to RFC 8684, 3.2. Starting a New Subflow,
+* we should use an "MPTCP specific error" reason code.
+*/
+ tcp6_request_sock_ops.send_reset(sk, skb,
SK_RST_REASON_MPTCP_RST_EMPTCP);
Same issue here.
return NULL;
}
#endif
@@ -778,6 +784,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock
*sk,
bool fallback, fallback_is_fatal;
struct mptcp_sock *owner;
struct sock *child;
+ int reason;
pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
@@ -833,7 +840,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock
*sk,
*/
if (!ctx || fallback) {
if (fallback_is_fatal) {
- subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
+ reason = MPTCP_RST_EMPTCP;
+ subflow_add_reset_reason(skb, reason);
goto dispose_child;
}
goto fallback;
@@ -861,7 +869,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock
*sk,
} else if (ctx->mp_join) {
owner = subflow_req->msk;
if (!owner) {
- subflow_add_reset_reason(skb,
MPTCP_RST_EPROHIBIT);
+ reason = MPTCP_RST_EPROHIBIT;
+ subflow_add_reset_reason(skb, reason);
goto dispose_child;
}
@@ -875,13 +884,18 @@ static struct sock *subflow_syn_recv_sock(const struct
sock *sk,
ntohs(inet_sk((struct sock
*)owner)->inet_sport));
if (!mptcp_pm_sport_in_anno_list(owner, sk)) {
SUBFLOW_REQ_INC_STATS(req,
MPTCP_MIB_MISMATCHPORTACKRX);
+ reason = MPTCP_RST_EUNSPEC;
I think the MPTCP code here should have been using MPTCP_RST_EPROHIBIT.
- Mat
goto dispose_child;
}
SUBFLOW_REQ_INC_STATS(req,
MPTCP_MIB_JOINPORTACKRX);
}
- if (!mptcp_finish_join(child))
+ if (!mptcp_finish_join(child)) {
+ struct mptcp_subflow_context *subflow =
mptcp_subflow_ctx(sk);
+
+ reason = subflow->reset_reason;
goto dispose_child;
+ }
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
tcp_rsk(req)->drop_req = true;
@@ -901,7 +915,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock
*sk,
tcp_rsk(req)->drop_req = true;
inet_csk_prepare_for_destroy_sock(child);
tcp_done(child);
- req->rsk_ops->send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+ req->rsk_ops->send_reset(sk, skb, convert_mptcp_reason(reason));
/* The last child reference will be released by the caller */
return child;
--
2.37.3
Re: [MPTCP][PATCH net-next] mptcp: clear use_ack and use_map when dropping other suboptions
13: R14: 88807f6ddd00 R15: 0002 [ 15.259822] FS: 7fae4a60a700() GS:88807c50() knlGS: [ 15.259826] CS: 0010 DS: ES: CR0: 80050033 [ 15.260296] RDX: RSI: 818e06c5 RDI: 88807f6dc700 [ 15.262514] CR2: 0048 CR3: 0b89c000 CR4: 06e0 [ 15.262515] Call Trace: [ 15.262519] skb_release_all+0x28/0x30 [ 15.262523] __kfree_skb+0x11/0x20 [ 15.263054] RBP: 88807f71a4c0 R08: 0001 R09: 0001 [ 15.263680] tcp_data_queue+0x270/0x1240 [ 15.263843] R10: c900019c7c18 R11: R12: 88807f71a4f0 [ 15.264693] ? tcp_urg+0x50/0x2a0 [ 15.264856] R13: R14: 88807f6dc700 R15: 0002 [ 15.265720] tcp_rcv_established+0x39a/0x890 [ 15.266438] FS: 7f65d9b5f700() GS:88807c40() knlGS: [ 15.267283] ? __schedule+0x3fa/0x880 [ 15.267287] tcp_v4_do_rcv+0xb9/0x270 [ 15.267290] __release_sock+0x8a/0x160 [ 15.268049] CS: 0010 DS: ES: CR0: 80050033 [ 15.268788] release_sock+0x32/0xd0 [ 15.268791] __inet_stream_connect+0x1d2/0x400 [ 15.268795] ? do_wait_intr_irq+0x80/0x80 [ 15.269593] CR2: 00223b10 CR3: 0b883000 CR4: 06f0 [ 15.270246] inet_stream_connect+0x36/0x50 [ 15.270250] mptcp_stream_connect+0x69/0x1b0 [ 15.270253] __sys_connect+0x122/0x140 [ 15.271097] Kernel panic - not syncing: Fatal exception [ 15.271820] ? syscall_enter_from_user_mode+0x17/0x50 [ 15.283542] ? lockdep_hardirqs_on_prepare+0xd4/0x170 [ 15.284275] __x64_sys_connect+0x1a/0x20 [ 15.284853] do_syscall_64+0x33/0x40 [ 15.285369] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 15.286105] RIP: 0033:0x7fae49f19469 [ 15.286638] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ff 49 2b 00 f7 d8 64 89 01 48 [ 15.289295] RSP: 002b:7fae4a609da8 EFLAGS: 0246 ORIG_RAX: 002a [ 15.290375] RAX: ffda RBX: 0049bf00 RCX: 7fae49f19469 [ 15.291403] RDX: 0010 RSI: 20c0 RDI: 0005 [ 15.292437] RBP: 0049bf00 R08: R09: [ 15.293456] R10: R11: 0246 R12: 0049bf0c [ 15.294473] R13: 7fff0004b6bf R14: 7fae4a5ea000 R15: 0003 [ 15.295492] Modules linked in: [ 15.295944] CR2: 0048 [ 15.296567] Kernel Offset: disabled [ 15.296941] ---[ end Kernel panic - not syncing: Fatal exception ]--- Reported-by: Christoph Paasch Fixes: 84dfe3677a6f (mptcp: send out dedicated ADD_ADDR packet) Signed-off-by: Geliang Tang --- net/mptcp/options.c | 2 ++ 1 file changed, 2 insertions(+) David or Jakub, this patch is intended for the -net tree (not net-next as labeled in the subject line). If you can apply it to -net, that's great, otherwise it can be resubmitted as [PATCH net]. In any case, the content is good: Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [PATCH][next] mptcp: fix a dereference of pointer before msk is null checked.
On Mon, 9 Nov 2020, Colin King wrote:
From: Colin Ian King
Currently the assignment of pointer net from the sock_net(sk) call
is potentially dereferencing a null pointer sk. sk points to the
same location as pointer msk and msk is being null checked after
the sock_net call. Fix this by calling sock_net after the null
check on pointer msk.
Addresses-Coverity: ("Dereference before null check")
Fixes: 00cfd77b9063 ("mptcp: retransmit ADD_ADDR when timeout")
Signed-off-by: Colin Ian King
---
net/mptcp/pm_netlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Hi Colin and Jakub -
I noticed that the follow-up discussion on this patch didn't go to the
netdev list, so patchwork did not get updated.
This patch is superseded by the following, which already has a Reviewed-by
tag from Matthieu:
http://patchwork.ozlabs.org/project/netdev/patch/078a2ef5bdc4e3b2c25ef852461692001f426495.1604976945.git.geliangt...@gmail.com/
Thanks!
--
Mat Martineau
Intel
Re: [PATCH net] selftests: mptcp: depends on built-in IPv6
On Wed, 21 Oct 2020, Matthieu Baerts wrote:
Recently, CONFIG_MPTCP_IPV6 no longer selects CONFIG_IPV6. As a
consequence, if CONFIG_MPTCP_IPV6=y is added to the kconfig, it will no
longer ensure CONFIG_IPV6=y. If it is not enabled, CONFIG_MPTCP_IPV6
will stay disabled and selftests will fail.
We also need CONFIG_IPV6 to be built-in. For more details, please see
commit 0ed37ac586c0 ("mptcp: depends on IPV6 but not as a module").
Note that 'make kselftest-merge' will take all 'config' files found in
'tools/testsing/selftests'. Because some of them already set
CONFIG_IPV6=y, MPTCP selftests were still passing. But they will fail if
MPTCP selftests are launched manually after having executed this command
to prepare the kernel config:
./scripts/kconfig/merge_config.sh -m .config \
./tools/testing/selftests/net/mptcp/config
Fixes: 010b430d5df5 ("mptcp: MPTCP_IPV6 should depend on IPV6 instead of selecting
it")
Signed-off-by: Matthieu Baerts
---
tools/testing/selftests/net/mptcp/config | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Mat Martineau
--
Mat Martineau
Intel
Re: linux-next: manual merge of the net-next tree with the net tree
On Thu, 1 Oct 2020, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
net/mptcp/protocol.h
between commit:
1a49b2c2a501 ("mptcp: Handle incoming 32-bit DATA_FIN values")
from the net tree and commit:
5c8c1640956e ("mptcp: add mptcp_destroy_common helper")
from the net-next tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc net/mptcp/protocol.h
index 20f04ac85409,7cfe52aeb2b8..
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@@ -387,7 -407,8 +407,8 @@@ void mptcp_data_ready(struct sock *sk,
bool mptcp_finish_join(struct sock *sk);
void mptcp_data_acked(struct sock *sk);
void mptcp_subflow_eof(struct sock *sk);
-bool mptcp_update_rcv_data_fin(struct mptcp_sock *msk, u64 data_fin_seq);
+bool mptcp_update_rcv_data_fin(struct mptcp_sock *msk, u64 data_fin_seq, bool
use_64bit);
+ void mptcp_destroy_common(struct mptcp_sock *msk);
Yes, this is the appropriate conflict resolution. Thanks!
--
Mat Martineau
Intel
Re: linux-next: manual merge of the net-next tree with the net tree
ail(>sk_receive_queue);
+ if (tail && mptcp_try_coalesce(sk, tail, skb))
+ return true;
+
+ skb_set_owner_r(skb, sk);
+ __skb_queue_tail(>sk_receive_queue, skb);
return true;
+ } else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
+ mptcp_data_queue_ofo(msk, skb);
+ return false;
+ }
- subflow->data_avail = 0;
- return mptcp_subflow_data_available(ssk);
+ /* old data, keep it simple and drop the whole pkt, sender
+* will retransmit as needed, if needed.
+*/
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
+ mptcp_drop(sk, skb);
+ return false;
+ }
+
+ static void mptcp_stop_timer(struct sock *sk)
+ {
+ struct inet_connection_sock *icsk = inet_csk(sk);
+
+ sk_stop_timer(sk, >icsk_retransmit_timer);
+ mptcp_sk(sk)->timer_ival = 0;
}
static void mptcp_check_data_fin_ack(struct sock *sk)
--
Mat Martineau
Intel
Re: [MPTCP][PATCH net-next 15/16] mptcp: add sk_stop_timer_sync helper
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added a new helper sk_stop_timer_sync, it deactivates a timer like sk_stop_timer, but waits for the handler to finish. Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- include/net/sock.h | 2 ++ net/core/sock.c| 7 +++ 2 files changed, 9 insertions(+) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 16/16] mptcp: retransmit ADD_ADDR when timeout
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch implemented the retransmition of ADD_ADDR when no ADD_ADDR echo is received. It added a timer with the announced address. When timeout occurs, ADD_ADDR will be retransmitted. Suggested-by: Mat Martineau Suggested-by: Paolo Abeni Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/options.c| 1 + net/mptcp/pm_netlink.c | 109 ++--- net/mptcp/protocol.h | 3 ++ 3 files changed, 96 insertions(+), 17 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 14/16] mptcp: add struct mptcp_pm_add_entry
On Thu, 24 Sep 2020, Geliang Tang wrote: Add a new struct mptcp_pm_add_entry to describe add_addr's entry. Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/pm_netlink.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 13/16] selftests: mptcp: add remove addr and subflow test cases
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added the remove addr and subflow test cases and two new functions. The first function run_remove_tests calls do_transfer with two new arguments, rm_nr_ns1 and rm_nr_ns2, for the numbers of addresses should be removed during the transfer process in namespace 1 and namespace 2. If both these two arguments are 0, we do the join test cases with "mptcp_connect -j" command. Otherwise, do the remove test cases with "mptcp_connect -r" command. The second function chk_rm_nr checks the RM_ADDR related mibs's counters. The output of the test cases looks like this: 11 remove single subflow syn[ ok ] - synack[ ok ] - ack[ ok ] rm [ ok ] - sf[ ok ] 12 remove multiple subflowssyn[ ok ] - synack[ ok ] - ack[ ok ] rm [ ok ] - sf[ ok ] 13 remove single address syn[ ok ] - synack[ ok ] - ack[ ok ] add[ ok ] - echo [ ok ] rm [ ok ] - sf[ ok ] 14 remove subflow and signal syn[ ok ] - synack[ ok ] - ack[ ok ] add[ ok ] - echo [ ok ] rm [ ok ] - sf[ ok ] 15 remove subflows and signal syn[ ok ] - synack[ ok ] - ack[ ok ] add[ ok ] - echo [ ok ] rm [ ok ] - sf[ ok ] Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Suggested-by: Mat Martineau Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- .../testing/selftests/net/mptcp/mptcp_join.sh | 145 +- 1 file changed, 142 insertions(+), 3 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 12/16] selftests: mptcp: add remove cfg in mptcp_connect
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added a new cfg, named cfg_remove in mptcp_connect. This new cfg_remove is copied from cfg_join. The only difference between them is in the do_rnd_write function. Here we slow down the transfer process of all data to let the RM_ADDR suboption can be sent and received completely. Otherwise the remove address and subflow test cases don't work. Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Suggested-by: Mat Martineau Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- .../selftests/net/mptcp/mptcp_connect.c| 18 +++--- 1 file changed, 15 insertions(+), 3 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 11/16] mptcp: add mptcp_destroy_common helper
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added a new helper named mptcp_destroy_common containing the shared code between mptcp_destroy() and mptcp_sock_destruct(). Suggested-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/protocol.c | 11 --- net/mptcp/protocol.h | 1 + net/mptcp/subflow.c | 4 +--- 3 files changed, 10 insertions(+), 6 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 10/16] mptcp: add RM_ADDR related mibs
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added two new mibs for RM_ADDR, named MPTCP_MIB_RMADDR and MPTCP_MIB_RMSUBFLOW, when the RM_ADDR suboption is received, increase the first mib counter, when the local subflow is removed, increase the second mib counter. Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Suggested-by: Mat Martineau Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/mib.c| 2 ++ net/mptcp/mib.h| 2 ++ net/mptcp/pm_netlink.c | 5 + 3 files changed, 9 insertions(+) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 08/16] mptcp: remove addr and subflow in PM netlink
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch implements the remove announced addr and subflow logic in PM netlink. When the PM netlink removes an address, we traverse all the existing msk sockets to find the relevant sockets. We add a new list named anno_list in mptcp_pm_data, to record all the announced addrs. In the traversing, we check if it has been recorded. If it has been, we trigger the RM_ADDR signal. We also check if this address is in conn_list. If it is, we remove the subflow which using this local address. Since we call mptcp_pm_free_anno_list in mptcp_destroy, we need to move __mptcp_init_sock before the mptcp_is_enabled check in mptcp_init_sock. Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Suggested-by: Mat Martineau Acked-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/pm.c | 7 ++- net/mptcp/pm_netlink.c | 122 +++-- net/mptcp/protocol.c | 9 +-- net/mptcp/protocol.h | 2 + net/mptcp/subflow.c| 1 + 5 files changed, 130 insertions(+), 11 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 03/16] mptcp: add the incoming RM_ADDR support
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added the RM_ADDR option parsing logic: We parsed the incoming options to find if the rm_addr option is received, and called mptcp_pm_rm_addr_received to schedule PM work to a new status, named MPTCP_PM_RM_ADDR_RECEIVED. PM work got this status, and called mptcp_pm_nl_rm_addr_received to handle it. In mptcp_pm_nl_rm_addr_received, we closed the subflow matching the rm_id, and updated PM counter. Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Suggested-by: Mat Martineau Signed-off-by: Geliang Tang --- net/mptcp/options.c| 5 + net/mptcp/pm.c | 12 net/mptcp/pm_netlink.c | 34 ++ net/mptcp/protocol.c | 12 net/mptcp/protocol.h | 7 +++ 5 files changed, 66 insertions(+), 4 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 02/16] mptcp: add the outgoing RM_ADDR support
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch added a new signal named rm_addr_signal in PM. On outgoing path, we called mptcp_pm_should_rm_signal to check if rm_addr_signal has been set. If it has been, we sent out the RM_ADDR option. Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/options.c | 29 + net/mptcp/pm.c | 25 + net/mptcp/protocol.h | 9 + 3 files changed, 63 insertions(+) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: [MPTCP][PATCH net-next 01/16] mptcp: rename addr_signal and the related functions
On Thu, 24 Sep 2020, Geliang Tang wrote: This patch renamed addr_signal and the related functions with the explicit word "add". Suggested-by: Matthieu Baerts Suggested-by: Paolo Abeni Signed-off-by: Geliang Tang --- net/mptcp/options.c | 14 +++--- net/mptcp/pm.c | 12 ++-- net/mptcp/protocol.h | 10 +- 3 files changed, 18 insertions(+), 18 deletions(-) Reviewed-by: Mat Martineau -- Mat Martineau Intel
Re: Trying to run mptcp on my machine
On Mon, 31 Aug 2020, Eric Curtin wrote: Hi Guys, I've been trying to get mptcp up and running on my machine (xubuntu 20.04) with little joy. What I did was install 5,8,5 kernel from here: https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.8.5/amd64/ Reboot, tried a curl: curl http://www.multipath-tcp.org Nay, Nay, Nay, your have an old computer that does not speak MPTCP. Shame on you! Checked this flag: sudo cat /proc/sys/net/mptcp/enabled 1 Even tried to run this guy in the kernel repo with no joy mptcp_connect.sh. Any pointers to get mptcp running? I couldn't find too much documentation on how to configure it on GNU/Linux. Hi Eric - I think one helpful guide for you would be this recent post by Davide (dcara...@redhat.com on the cc list): https://developers.redhat.com/blog/2020/08/19/multipath-tcp-on-red-hat-enterprise-linux-8-3-from-0-to-1-subflows/ With curl, what you're seeing is that existing programs do continue to use regular TCP. The blog post has a section on one approach to making unmodified programs open sockets with IPPROTO_MPTCP. The MPTCP upstream community has a mailing list at mp...@lists.01.org and a wiki at https://github.com/multipath-tcp/mptcp_net-next/wiki - and we are working on more documentation with the kind of pointers you're looking for. Thanks for trying out MPTCP! -- Mat Martineau Intel
Re: [PATCH] mptcp: use list_first_entry_or_null
Hello Geliang,
On Fri, 12 Jun 2020, Geliang Tang wrote:
Use list_first_entry_or_null to simplify the code.
Signed-off-by: Geliang Tang
---
net/mptcp/protocol.h | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 86d265500cf6..55c65abcad64 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -234,10 +234,7 @@ static inline struct mptcp_data_frag *mptcp_rtx_head(const
struct sock *sk)
{
struct mptcp_sock *msk = mptcp_sk(sk);
- if (list_empty(>rtx_queue))
- return NULL;
-
- return list_first_entry(>rtx_queue, struct mptcp_data_frag, list);
+ return list_first_entry_or_null(>rtx_queue, struct
mptcp_data_frag, list);
}
struct mptcp_subflow_request_sock {
--
2.17.1
As Matthieu mentioned on your earlier patch, please submit patches to
netdev with either a [PATCH net] or [PATCH net-next] tag. In this case,
these are non-critical bug fixes so they should be targeted at net-next.
Note that net-next branch is currently closed to submissions due to the
v5.8 merge window. Please resubmit both MPTCP patches for net-next when
David announces that it is open again. The change does look ok but will
not be merged now.
Thanks for your patch,
--
Mat Martineau
Intel
Re: [PATCH 2/2] keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h
On Mon, 24 Sep 2018, David Howells wrote:
The keyctl_dh_params struct in uapi/linux/keyctl.h contains the symbol
"private" which means that the header file will cause compilation failure
if #included in to a C++ program. Further, the patch that added the same
struct to the keyutils package named the symbol "priv", not "private".
The previous attempt to fix this (commit 8a2336e549d3) did so by simply
renaming the kernel's copy of the field to dh_private, but this then breaks
existing userspace and as such has to be reverted (which is done by the
preceding patch).
[And note, to those who think that wrapping the struct in extern "C" {}
will work: it won't; that only changes how symbol names are presented to
the assembler and linker.].
Instead, insert an anonymous union around the "private" member and add a
second member in there with the name "priv" to match the one in the
keyutils package. The "private" member is then wrapped in !__cplusplus
cpp-conditionals to hide it from C++.
Fixes: ddbb41148724 ("KEYS: Add KEYCTL_DH_COMPUTE command")
Fixes: 8a2336e549d3 ("uapi/linux/keyctl.h: don't use C++ reserved keyword as a
struct member name")
Signed-off-by: David Howells
cc: Randy Dunlap
cc: Lubomir Rintel
cc: James Morris
cc: Mat Martineau
cc: Stephan Mueller
cc: Andrew Morton
cc: Linus Torvalds
cc: sta...@vger.kernel.org
---
include/uapi/linux/keyctl.h |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 7b8c9e19bad1..0f3cb13db8e9 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -65,7 +65,12 @@
/* keyctl structures */
struct keyctl_dh_params {
- __s32 private;
+ union {
+#ifndef __cplusplus
+ __s32 private;
+#endif
+ __s32 priv;
+ };
__s32 prime;
__s32 base;
};
Thanks David. This is an important fix for existing C userspace code, it
would create a headache now and in to the future if the keyctl.h change
introduced in v4.19-rc3 made it in to an official release. Please merge,
James!
Reviewed-By: Mat Martineau
--
Mat Martineau
Intel OTC
Re: [PATCH 2/2] keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h
On Mon, 24 Sep 2018, David Howells wrote:
The keyctl_dh_params struct in uapi/linux/keyctl.h contains the symbol
"private" which means that the header file will cause compilation failure
if #included in to a C++ program. Further, the patch that added the same
struct to the keyutils package named the symbol "priv", not "private".
The previous attempt to fix this (commit 8a2336e549d3) did so by simply
renaming the kernel's copy of the field to dh_private, but this then breaks
existing userspace and as such has to be reverted (which is done by the
preceding patch).
[And note, to those who think that wrapping the struct in extern "C" {}
will work: it won't; that only changes how symbol names are presented to
the assembler and linker.].
Instead, insert an anonymous union around the "private" member and add a
second member in there with the name "priv" to match the one in the
keyutils package. The "private" member is then wrapped in !__cplusplus
cpp-conditionals to hide it from C++.
Fixes: ddbb41148724 ("KEYS: Add KEYCTL_DH_COMPUTE command")
Fixes: 8a2336e549d3 ("uapi/linux/keyctl.h: don't use C++ reserved keyword as a
struct member name")
Signed-off-by: David Howells
cc: Randy Dunlap
cc: Lubomir Rintel
cc: James Morris
cc: Mat Martineau
cc: Stephan Mueller
cc: Andrew Morton
cc: Linus Torvalds
cc: sta...@vger.kernel.org
---
include/uapi/linux/keyctl.h |7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 7b8c9e19bad1..0f3cb13db8e9 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -65,7 +65,12 @@
/* keyctl structures */
struct keyctl_dh_params {
- __s32 private;
+ union {
+#ifndef __cplusplus
+ __s32 private;
+#endif
+ __s32 priv;
+ };
__s32 prime;
__s32 base;
};
Thanks David. This is an important fix for existing C userspace code, it
would create a headache now and in to the future if the keyctl.h change
introduced in v4.19-rc3 made it in to an official release. Please merge,
James!
Reviewed-By: Mat Martineau
--
Mat Martineau
Intel OTC
Re: [PATCH] uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name
On Tue, 10 Apr 2018, David Howells wrote:
Randy Dunlap <rdun...@infradead.org> wrote:
Since this header is in "include/uapi/linux/", apparently people
want to use it in userspace programs -- even in C++ ones.
However, the header uses a C++ reserved keyword ("private"),
so change that to "dh_private" instead to allow the header file
to be used in C++ userspace.
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=191051
Ugh. Yeah. This is a UAPI breaker, but I think we have to do it, despite it
being 2 years old. Maybe wrap that element in a #ifdef so it's still allowed
in C?
cc'ing Mat Martineau as he's the originator of the structure.
I agree with David's assessment.
The keyctl() system call wrapper is implemented in libkeyutils, which may
reduce the need for the proposed ifdef. libkeyutils and its users don't
require any updates if this patch is merged because it has its own
keyword-free structure definition.
--
Mat Martineau
Intel OTC
Re: [PATCH] uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name
On Tue, 10 Apr 2018, David Howells wrote:
Randy Dunlap wrote:
Since this header is in "include/uapi/linux/", apparently people
want to use it in userspace programs -- even in C++ ones.
However, the header uses a C++ reserved keyword ("private"),
so change that to "dh_private" instead to allow the header file
to be used in C++ userspace.
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=191051
Ugh. Yeah. This is a UAPI breaker, but I think we have to do it, despite it
being 2 years old. Maybe wrap that element in a #ifdef so it's still allowed
in C?
cc'ing Mat Martineau as he's the originator of the structure.
I agree with David's assessment.
The keyctl() system call wrapper is implemented in libkeyutils, which may
reduce the need for the proposed ifdef. libkeyutils and its users don't
require any updates if this patch is merged because it has its own
keyword-free structure definition.
--
Mat Martineau
Intel OTC
Re: [PATCH] KEYS: reject NULL restriction string when type is specified
On Fri, 8 Dec 2017, David Howells wrote: Mat Martineau <mathew.j.martin...@linux.intel.com> wrote: Since this fixes the bug for the asymmetric key type and ensures that other key types won't make the same mistake, I agree this is the way to fix it. I did not find any issues in the patch. Can I put that down as a Reviewed-by? Yes. Looks like I missed the window for your pull request, though - I'll be sure to add Reviewed-by in future reviews. -- Mat Martineau Intel OTC
Re: [PATCH] KEYS: reject NULL restriction string when type is specified
On Fri, 8 Dec 2017, David Howells wrote: Mat Martineau wrote: Since this fixes the bug for the asymmetric key type and ensures that other key types won't make the same mistake, I agree this is the way to fix it. I did not find any issues in the patch. Can I put that down as a Reviewed-by? Yes. Looks like I missed the window for your pull request, though - I'll be sure to add Reviewed-by in future reviews. -- Mat Martineau Intel OTC
Re: [PATCH] KEYS: reject NULL restriction string when type is specified
Eric,
On Thu, 30 Nov 2017, Eric Biggers wrote:
From: Eric Biggers <ebigg...@google.com>
keyctl_restrict_keyring() allows through a NULL restriction when the
"type" is non-NULL, which causes a NULL pointer dereference in
asymmetric_lookup_restriction() when it calls strcmp() on the
restriction string.
But no key types actually use a "NULL restriction" to mean anything, so
update keyctl_restrict_keyring() to reject it with EINVAL.
Since this fixes the bug for the asymmetric key type and ensures that
other key types won't make the same mistake, I agree this is the way to
fix it. I did not find any issues in the patch.
Thanks,
Mat
Reported-by: syzbot <syzkal...@googlegroups.com>
Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key
type")
Cc: <sta...@vger.kernel.org> # v4.12+
Signed-off-by: Eric Biggers <ebigg...@google.com>
---
security/keys/keyctl.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 76d22f726ae4..1ffe60bb2845 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1588,9 +1588,8 @@ long keyctl_session_to_parent(void)
* The caller must have Setattr permission to change keyring restrictions.
*
* The requested type name may be a NULL pointer to reject all attempts
- * to link to the keyring. If _type is non-NULL, _restriction can be
- * NULL or a pointer to a string describing the restriction. If _type is
- * NULL, _restriction must also be NULL.
+ * to link to the keyring. In this case, _restriction must also be NULL.
+ * Otherwise, both _type and _restriction must be non-NULL.
*
* Returns 0 if successful.
*/
@@ -1598,7 +1597,6 @@ long keyctl_restrict_keyring(key_serial_t id, const char
__user *_type,
const char __user *_restriction)
{
key_ref_t key_ref;
- bool link_reject = !_type;
char type[32];
char *restriction = NULL;
long ret;
@@ -1607,31 +1605,29 @@ long keyctl_restrict_keyring(key_serial_t id, const
char __user *_type,
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
+ ret = -EINVAL;
if (_type) {
- ret = key_get_type_from_user(type, _type, sizeof(type));
- if (ret < 0)
+ if (!_restriction)
goto error;
- }
- if (_restriction) {
- if (!_type) {
- ret = -EINVAL;
+ ret = key_get_type_from_user(type, _type, sizeof(type));
+ if (ret < 0)
goto error;
- }
restriction = strndup_user(_restriction, PAGE_SIZE);
if (IS_ERR(restriction)) {
ret = PTR_ERR(restriction);
goto error;
}
+ } else {
+ if (_restriction)
+ goto error;
}
- ret = keyring_restrict(key_ref, link_reject ? NULL : type, restriction);
+ ret = keyring_restrict(key_ref, _type ? type : NULL, restriction);
kfree(restriction);
-
error:
key_ref_put(key_ref);
-
return ret;
}
--
2.15.0.531.g2ccb3012c9-goog
--
Mat Martineau
Intel OTC
Re: [PATCH] KEYS: reject NULL restriction string when type is specified
Eric,
On Thu, 30 Nov 2017, Eric Biggers wrote:
From: Eric Biggers
keyctl_restrict_keyring() allows through a NULL restriction when the
"type" is non-NULL, which causes a NULL pointer dereference in
asymmetric_lookup_restriction() when it calls strcmp() on the
restriction string.
But no key types actually use a "NULL restriction" to mean anything, so
update keyctl_restrict_keyring() to reject it with EINVAL.
Since this fixes the bug for the asymmetric key type and ensures that
other key types won't make the same mistake, I agree this is the way to
fix it. I did not find any issues in the patch.
Thanks,
Mat
Reported-by: syzbot
Fixes: 97d3aa0f3134 ("KEYS: Add a lookup_restriction function for the asymmetric key
type")
Cc: # v4.12+
Signed-off-by: Eric Biggers
---
security/keys/keyctl.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 76d22f726ae4..1ffe60bb2845 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1588,9 +1588,8 @@ long keyctl_session_to_parent(void)
* The caller must have Setattr permission to change keyring restrictions.
*
* The requested type name may be a NULL pointer to reject all attempts
- * to link to the keyring. If _type is non-NULL, _restriction can be
- * NULL or a pointer to a string describing the restriction. If _type is
- * NULL, _restriction must also be NULL.
+ * to link to the keyring. In this case, _restriction must also be NULL.
+ * Otherwise, both _type and _restriction must be non-NULL.
*
* Returns 0 if successful.
*/
@@ -1598,7 +1597,6 @@ long keyctl_restrict_keyring(key_serial_t id, const char
__user *_type,
const char __user *_restriction)
{
key_ref_t key_ref;
- bool link_reject = !_type;
char type[32];
char *restriction = NULL;
long ret;
@@ -1607,31 +1605,29 @@ long keyctl_restrict_keyring(key_serial_t id, const
char __user *_type,
if (IS_ERR(key_ref))
return PTR_ERR(key_ref);
+ ret = -EINVAL;
if (_type) {
- ret = key_get_type_from_user(type, _type, sizeof(type));
- if (ret < 0)
+ if (!_restriction)
goto error;
- }
- if (_restriction) {
- if (!_type) {
- ret = -EINVAL;
+ ret = key_get_type_from_user(type, _type, sizeof(type));
+ if (ret < 0)
goto error;
- }
restriction = strndup_user(_restriction, PAGE_SIZE);
if (IS_ERR(restriction)) {
ret = PTR_ERR(restriction);
goto error;
}
+ } else {
+ if (_restriction)
+ goto error;
}
- ret = keyring_restrict(key_ref, link_reject ? NULL : type, restriction);
+ ret = keyring_restrict(key_ref, _type ? type : NULL, restriction);
kfree(restriction);
-
error:
key_ref_put(key_ref);
-
return ret;
}
--
2.15.0.531.g2ccb3012c9-goog
--
Mat Martineau
Intel OTC
Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
On Wed, 8 Nov 2017, Eric Biggers wrote: On Wed, Nov 08, 2017 at 08:17:12PM +, David Howells wrote: Eric Biggers <ebigge...@gmail.com> wrote: This probably should be grouped with my series "crypto: dh - input validation fixes", as this is also a fix for Diffie-Hellman. I was actually expecting Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto API (crypto/dh.c); none of the patches actually touch security/keys/. Fine by me. If you'd like to maintain the Diffie-Hellman implementation(s) you should get yourself added to MAINTAINERS for the relevant files. It touches the MPI lib, though, so it also potentially affects RSA and stuff, which is why I introduced it to the kernel originally. True, mpi_powm() might also be reachable by adding a key of type "asymmetric", in which case this fix would need to go to older kernels than v4.12. If I have a chance I'll see if I can find a reproducer. CONFIG_KEY_DH_OPERATIONS and use of mpi_powm() by KEYCTL_DH_COMPUTE goes back to v4.7, when the MPI library was called directly. KPP was not implemented yet. -- Mat Martineau Intel OTC
Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
On Wed, 8 Nov 2017, Eric Biggers wrote: On Wed, Nov 08, 2017 at 08:17:12PM +, David Howells wrote: Eric Biggers wrote: This probably should be grouped with my series "crypto: dh - input validation fixes", as this is also a fix for Diffie-Hellman. I was actually expecting Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto API (crypto/dh.c); none of the patches actually touch security/keys/. Fine by me. If you'd like to maintain the Diffie-Hellman implementation(s) you should get yourself added to MAINTAINERS for the relevant files. It touches the MPI lib, though, so it also potentially affects RSA and stuff, which is why I introduced it to the kernel originally. True, mpi_powm() might also be reachable by adding a key of type "asymmetric", in which case this fix would need to go to older kernels than v4.12. If I have a chance I'll see if I can find a reproducer. CONFIG_KEY_DH_OPERATIONS and use of mpi_powm() by KEYCTL_DH_COMPUTE goes back to v4.7, when the MPI library was called directly. KPP was not implemented yet. -- Mat Martineau Intel OTC
Re: [PATCH] lib/mpi: call cond_resched() from mpi_powm() loop
Eric, On Mon, 6 Nov 2017, Eric Biggers wrote: From: Eric Biggers <ebigg...@google.com> On a non-preemptible kernel, if KEYCTL_DH_COMPUTE is called with the largest permitted inputs (16384 bits), the kernel spends 10+ seconds doing modular exponentiation in mpi_powm() without rescheduling. If all threads do it, it locks up the system. Moreover, it can cause rcu_sched-stall warnings. Notwithstanding the insanity of doing this calculation in kernel mode rather than in userspace, fix it by calling cond_resched() as each bit from the exponent is processed. It's still noninterruptible, but at least it's preemptible now. cond_resched() is in the outer loop and gets called every BITS_PER_LONG bits. That seems to be often enough for the system that was taking 10+ seconds, and might be ok for slower processors. Was your intent to call cond_resched() for every bit as you described in the commit message? Thanks for the fix. Mat Cc: sta...@vger.kernel.org # v4.12+ Signed-off-by: Eric Biggers <ebigg...@google.com> --- lib/mpi/mpi-pow.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/mpi/mpi-pow.c b/lib/mpi/mpi-pow.c index e24388a863a7..f089a52dbbdb 100644 --- a/lib/mpi/mpi-pow.c +++ b/lib/mpi/mpi-pow.c @@ -26,6 +26,7 @@ * however I decided to publish this code under the plain GPL. */ +#include #include #include "mpi-internal.h" #include "longlong.h" @@ -263,6 +264,8 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod) break; e = ep[i]; c = BITS_PER_MPI_LIMB; + + cond_resched(); } /* We shifted MOD, the modulo reduction argument, left MOD_SHIFT_CNT -- 2.15.0 -- Mat Martineau Intel OTC
Re: [PATCH] lib/mpi: call cond_resched() from mpi_powm() loop
Eric, On Mon, 6 Nov 2017, Eric Biggers wrote: From: Eric Biggers On a non-preemptible kernel, if KEYCTL_DH_COMPUTE is called with the largest permitted inputs (16384 bits), the kernel spends 10+ seconds doing modular exponentiation in mpi_powm() without rescheduling. If all threads do it, it locks up the system. Moreover, it can cause rcu_sched-stall warnings. Notwithstanding the insanity of doing this calculation in kernel mode rather than in userspace, fix it by calling cond_resched() as each bit from the exponent is processed. It's still noninterruptible, but at least it's preemptible now. cond_resched() is in the outer loop and gets called every BITS_PER_LONG bits. That seems to be often enough for the system that was taking 10+ seconds, and might be ok for slower processors. Was your intent to call cond_resched() for every bit as you described in the commit message? Thanks for the fix. Mat Cc: sta...@vger.kernel.org # v4.12+ Signed-off-by: Eric Biggers --- lib/mpi/mpi-pow.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/mpi/mpi-pow.c b/lib/mpi/mpi-pow.c index e24388a863a7..f089a52dbbdb 100644 --- a/lib/mpi/mpi-pow.c +++ b/lib/mpi/mpi-pow.c @@ -26,6 +26,7 @@ * however I decided to publish this code under the plain GPL. */ +#include #include #include "mpi-internal.h" #include "longlong.h" @@ -263,6 +264,8 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod) break; e = ep[i]; c = BITS_PER_MPI_LIMB; + + cond_resched(); } /* We shifted MOD, the modulo reduction argument, left MOD_SHIFT_CNT -- 2.15.0 -- Mat Martineau Intel OTC
Re: [PATCH 6/9] efi: Add EFI signature data types
David,
On Wed, 16 Nov 2016, David Howells wrote:
From: Dave Howells <dhowe...@redhat.com>
Add the data types that are used for containing hashes, keys and
certificates for cryptographic verification along with their corresponding
type GUIDs.
Signed-off-by: David Howells <dhowe...@redhat.com>
---
include/linux/efi.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index acfdea8381de..6fb50bafedc7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -594,6 +594,9 @@ void efi_native_runtime_setup(void);
#define EFI_IMAGE_SECURITY_DATABASE_GUIDEFI_GUID(0xd719b2cb, 0x3d3a,
0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
#define EFI_SHIM_LOCK_GUID EFI_GUID(0x605dab50, 0xe046,
0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+#define EFI_CERT_SHA256_GUID EFI_GUID(0xc1c41626, 0x504c,
0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
+#define EFI_CERT_X509_GUID EFI_GUID(0xa5c059a1, 0x94e4,
0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
+#define EFI_CERT_X509_SHA256_GUID EFI_GUID(0x3bd2a492, 0x96c0,
0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed );
The trailing ';' on EFI_CERT_X509_SHA256_GUID could trip someone up later.
As long as you're fixing that, the spaces before the closing parens are
inconsistent with the other GUID definitions in this file.
/*
* This GUID is used to pass to the kernel proper the struct screen_info
@@ -853,6 +856,27 @@ typedef struct {
efi_memory_desc_t entry[0];
} efi_memory_attributes_table_t;
+typedef struct {
+ efi_guid_t signature_owner;
+ u8 signature_data[];
+} efi_signature_data_t;
+
+typedef struct {
+ efi_guid_t signature_type;
+ u32 signature_list_size;
+ u32 signature_header_size;
+ u32 signature_size;
+ u8 signature_header[];
+ /* efi_signature_data_t signatures[][] */
+} efi_signature_list_t;
+
+typedef u8 efi_sha256_hash_t[32];
+
+typedef struct {
+ efi_sha256_hash_t to_be_signed_hash;
+ efi_time_t time_of_revocation;
+} efi_cert_x509_sha256_t;
+
/*
* All runtime access to EFI goes through this structure:
*/
--
Mat Martineau
Intel OTC
Re: [PATCH 6/9] efi: Add EFI signature data types
David,
On Wed, 16 Nov 2016, David Howells wrote:
From: Dave Howells
Add the data types that are used for containing hashes, keys and
certificates for cryptographic verification along with their corresponding
type GUIDs.
Signed-off-by: David Howells
---
include/linux/efi.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index acfdea8381de..6fb50bafedc7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -594,6 +594,9 @@ void efi_native_runtime_setup(void);
#define EFI_IMAGE_SECURITY_DATABASE_GUIDEFI_GUID(0xd719b2cb, 0x3d3a,
0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
#define EFI_SHIM_LOCK_GUID EFI_GUID(0x605dab50, 0xe046,
0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+#define EFI_CERT_SHA256_GUID EFI_GUID(0xc1c41626, 0x504c,
0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
+#define EFI_CERT_X509_GUID EFI_GUID(0xa5c059a1, 0x94e4,
0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
+#define EFI_CERT_X509_SHA256_GUID EFI_GUID(0x3bd2a492, 0x96c0,
0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed );
The trailing ';' on EFI_CERT_X509_SHA256_GUID could trip someone up later.
As long as you're fixing that, the spaces before the closing parens are
inconsistent with the other GUID definitions in this file.
/*
* This GUID is used to pass to the kernel proper the struct screen_info
@@ -853,6 +856,27 @@ typedef struct {
efi_memory_desc_t entry[0];
} efi_memory_attributes_table_t;
+typedef struct {
+ efi_guid_t signature_owner;
+ u8 signature_data[];
+} efi_signature_data_t;
+
+typedef struct {
+ efi_guid_t signature_type;
+ u32 signature_list_size;
+ u32 signature_header_size;
+ u32 signature_size;
+ u8 signature_header[];
+ /* efi_signature_data_t signatures[][] */
+} efi_signature_list_t;
+
+typedef u8 efi_sha256_hash_t[32];
+
+typedef struct {
+ efi_sha256_hash_t to_be_signed_hash;
+ efi_time_t time_of_revocation;
+} efi_cert_x509_sha256_t;
+
/*
* All runtime access to EFI goes through this structure:
*/
--
Mat Martineau
Intel OTC
Re: [PATCH 2/8] KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]
On Thu, 23 Jun 2016, David Howells wrote:
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 8ac2c5fbc8fc..93ebd25b1427 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -60,6 +60,11 @@
#define KEYCTL_INVALIDATE 21 /* invalidate a key */
#define KEYCTL_GET_PERSISTENT 22 /* get a user's persistent
keyring */
#define KEYCTL_DH_COMPUTE 23 /* Compute Diffie-Hellman
values */
+#define KEYCTL_PKEY_QUERY 24 /* Query public key parameters
*/
+#define KEYCTL_PKEY_ENCRYPT25 /* Encrypt a blob using a
public key */
+#define KEYCTL_PKEY_DECRYPT26 /* Decrypt a blob using a
public key */
+#define KEYCTL_PKEY_SIGN 27 /* Create a public key
signature */
+#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key
signature */
/* keyctl structures */
struct keyctl_dh_params {
@@ -73,4 +78,24 @@ struct keyctl_dh_params {
#define KEYCTL_SUPPORTS_SIGN0x04
#define KEYCTL_SUPPORTS_VERIFY 0x08
+struct keyctl_pkey_query {
+ __u32 supported_ops; /* Which ops are supported */
+ __u32 key_size; /* Size of the key in bits */
+ __u16 max_data_size; /* Maximum size of raw data to sign in
bytes */
+ __u16 max_sig_size; /* Maximum size of signature in bytes */
+ __u16 max_enc_size; /* Maximum size of encrypted blob in
bytes */
+ __u16 max_dec_size; /* Maximum size of decrypted blob in
bytes */
+ __u32 __spare[10];
+};
It would also be useful to return pkey_algo so userspace can see which
algorithm is in use for the given public key. The public key algorithm is
printed in /proc/keys, but is not returned by KEYCTL_PKEY_QUERY or
KEYCTL_DESCRIBE.
Does it make sense to add the information from key->type->describe() to
KEYCTL_PKEY_QUERY or KEYCTL_DESCRIBE? Or add something new like
KEYCTL_DESCRIBE_TYPE?
--
Mat Martineau
Intel OTC
Re: [PATCH 2/8] KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver #2]
On Thu, 23 Jun 2016, David Howells wrote:
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 8ac2c5fbc8fc..93ebd25b1427 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -60,6 +60,11 @@
#define KEYCTL_INVALIDATE 21 /* invalidate a key */
#define KEYCTL_GET_PERSISTENT 22 /* get a user's persistent
keyring */
#define KEYCTL_DH_COMPUTE 23 /* Compute Diffie-Hellman
values */
+#define KEYCTL_PKEY_QUERY 24 /* Query public key parameters
*/
+#define KEYCTL_PKEY_ENCRYPT25 /* Encrypt a blob using a
public key */
+#define KEYCTL_PKEY_DECRYPT26 /* Decrypt a blob using a
public key */
+#define KEYCTL_PKEY_SIGN 27 /* Create a public key
signature */
+#define KEYCTL_PKEY_VERIFY 28 /* Verify a public key
signature */
/* keyctl structures */
struct keyctl_dh_params {
@@ -73,4 +78,24 @@ struct keyctl_dh_params {
#define KEYCTL_SUPPORTS_SIGN0x04
#define KEYCTL_SUPPORTS_VERIFY 0x08
+struct keyctl_pkey_query {
+ __u32 supported_ops; /* Which ops are supported */
+ __u32 key_size; /* Size of the key in bits */
+ __u16 max_data_size; /* Maximum size of raw data to sign in
bytes */
+ __u16 max_sig_size; /* Maximum size of signature in bytes */
+ __u16 max_enc_size; /* Maximum size of encrypted blob in
bytes */
+ __u16 max_dec_size; /* Maximum size of decrypted blob in
bytes */
+ __u32 __spare[10];
+};
It would also be useful to return pkey_algo so userspace can see which
algorithm is in use for the given public key. The public key algorithm is
printed in /proc/keys, but is not returned by KEYCTL_PKEY_QUERY or
KEYCTL_DESCRIBE.
Does it make sense to add the information from key->type->describe() to
KEYCTL_PKEY_QUERY or KEYCTL_DESCRIBE? Or add something new like
KEYCTL_DESCRIBE_TYPE?
--
Mat Martineau
Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
On Fri, 8 Jul 2016, Tadeusz Struk wrote: Hi Mat, On 07/06/2016 12:38 PM, Mat Martineau wrote: So it looks like the only thing that we need to return to the user in this case is the return code. Do you agree? The way verify_signature is implemented today, the only output is the return code. For verify, maybe no read is required (just sendmsg() and check the return code). But this isn't the extent of the problem: verify_signature needs both the signature to be verified and the expected hash as inputs. How is the expected hash provided? Would you include it as a cmsg header? ALG_OP_VERIFY should have consistent inputs and outputs whether the key was set with ALG_SET_KEY_ID or ALG_SET_KEY. The signature of verify_signature() is quite different from the other new public key handlers, i.e. create_signature(), encrypt_blob(), and decrypt_blob(). For verify_signature() we need the following parameters: encrypted src, hash function to use, expected digest. The expected digest could be optional if we would modify the verify_signature() to return the decrypted buffer. I think the best solution for now would be to just return -ENOPROTOOPT for verify_signature in SET_KEY_ID mode. All the four operations will be supported in the SET_KEY mode and all but verify_signature() will be supported in the SET_KEY_ID mode. This can added later if we will find a way to pass all parameters in a consistent way. What do you think? If you are ok with that I will send a new version soon. Are the inputs and outputs defined for ALG_OP_VERIFY in SET_KEY mode going to work for hardware keys (like TPM) in SET_KEY_ID mode? That's needed if the verify SET_KEY_ID mode is to be added later. -- Mat Martineau Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
On Fri, 8 Jul 2016, Tadeusz Struk wrote: Hi Mat, On 07/06/2016 12:38 PM, Mat Martineau wrote: So it looks like the only thing that we need to return to the user in this case is the return code. Do you agree? The way verify_signature is implemented today, the only output is the return code. For verify, maybe no read is required (just sendmsg() and check the return code). But this isn't the extent of the problem: verify_signature needs both the signature to be verified and the expected hash as inputs. How is the expected hash provided? Would you include it as a cmsg header? ALG_OP_VERIFY should have consistent inputs and outputs whether the key was set with ALG_SET_KEY_ID or ALG_SET_KEY. The signature of verify_signature() is quite different from the other new public key handlers, i.e. create_signature(), encrypt_blob(), and decrypt_blob(). For verify_signature() we need the following parameters: encrypted src, hash function to use, expected digest. The expected digest could be optional if we would modify the verify_signature() to return the decrypted buffer. I think the best solution for now would be to just return -ENOPROTOOPT for verify_signature in SET_KEY_ID mode. All the four operations will be supported in the SET_KEY mode and all but verify_signature() will be supported in the SET_KEY_ID mode. This can added later if we will find a way to pass all parameters in a consistent way. What do you think? If you are ok with that I will send a new version soon. Are the inputs and outputs defined for ALG_OP_VERIFY in SET_KEY mode going to work for hardware keys (like TPM) in SET_KEY_ID mode? That's needed if the verify SET_KEY_ID mode is to be added later. -- Mat Martineau Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
On Tue, 5 Jul 2016, Tadeusz Struk wrote:
Hi Mat,
On 06/29/2016 11:43 AM, Mat Martineau wrote:
+ret = verify_signature(key, );
+if (!ret) {
+req->dst_len = sizeof(digest);
I think you fixed the BUG_ON() problem but there's still an issue with
the handling of the digest. Check the use of sig->digest in
public_key_verify_signature(), it's an input not an output. Right now it
looks like 20 uninitialized bytes are compared with the computed digest
within verify_signature, and then the unintialized bytes are copied to
req->dst here.
With some modifications to public_key_verify_signature you could get the
digest you need, but I'm not sure if verification with a hardware key
(like a key in a TPM) can or can not provide the digest needed. Maybe
this is why the verify_signature hook in struct asymmetric_key_subtype
is optional.
+scatterwalk_map_and_copy(digest, req->dst, 0, req->dst_len, 1);
+}
So it looks like the only thing that we need to return to the user in
this case is the return code. Do you agree?
The way verify_signature is implemented today, the only output is the
return code. For verify, maybe no read is required (just sendmsg() and
check the return code).
But this isn't the extent of the problem: verify_signature needs both the
signature to be verified and the expected hash as inputs. How is the
expected hash provided? Would you include it as a cmsg header?
ALG_OP_VERIFY should have consistent inputs and outputs whether the key
was set with ALG_SET_KEY_ID or ALG_SET_KEY.
--
Mat Martineau
Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
On Tue, 5 Jul 2016, Tadeusz Struk wrote:
Hi Mat,
On 06/29/2016 11:43 AM, Mat Martineau wrote:
+ret = verify_signature(key, );
+if (!ret) {
+req->dst_len = sizeof(digest);
I think you fixed the BUG_ON() problem but there's still an issue with
the handling of the digest. Check the use of sig->digest in
public_key_verify_signature(), it's an input not an output. Right now it
looks like 20 uninitialized bytes are compared with the computed digest
within verify_signature, and then the unintialized bytes are copied to
req->dst here.
With some modifications to public_key_verify_signature you could get the
digest you need, but I'm not sure if verification with a hardware key
(like a key in a TPM) can or can not provide the digest needed. Maybe
this is why the verify_signature hook in struct asymmetric_key_subtype
is optional.
+scatterwalk_map_and_copy(digest, req->dst, 0, req->dst_len, 1);
+}
So it looks like the only thing that we need to return to the user in
this case is the return code. Do you agree?
The way verify_signature is implemented today, the only output is the
return code. For verify, maybe no read is required (just sendmsg() and
check the return code).
But this isn't the extent of the problem: verify_signature needs both the
signature to be verified and the expected hash as inputs. How is the
expected hash provided? Would you include it as a cmsg header?
ALG_OP_VERIFY should have consistent inputs and outputs whether the key
was set with ALG_SET_KEY_ID or ALG_SET_KEY.
--
Mat Martineau
Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
Tadeusz,
On Thu, 23 Jun 2016, Tadeusz Struk wrote:
This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options are
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID for setting public and
private keys respectively. When these new options will be used
the user, instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.
When a key corresponding to the given keyid is found, it is stored
in the socket context and subsequent crypto operation invoked by the
user will use the new asymmetric accessor functions instead of akcipher
api. The asymmetric subtype can internally use akcipher api or
invoke operations defined by a given subtype, depending on the
key type.
Signed-off-by: Tadeusz Struk <tadeusz.st...@intel.com>
---
crypto/af_alg.c | 10 ++
crypto/algif_akcipher.c | 212 ++-
include/crypto/if_alg.h |1
include/uapi/linux/if_alg.h |2
4 files changed, 220 insertions(+), 5 deletions(-)
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index 2b8d37e..106f715 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
+static int asym_key_verify(const struct key *key, struct akcipher_request *req)
+{
+ struct public_key_signature sig;
+ char *src = NULL, *in, digest[20];
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ sig.pkey_algo = "rsa";
+ sig.encoding = "pkcs1";
+ /* Need to find a way to pass the hash param */
Comment still needed?
+ sig.hash_algo = "sha1";
+ sig.digest_size = sizeof(digest);
+ sig.digest = digest;
+ sig.s_size = req->src_len;
+ sig.s = src;
+ ret = verify_signature(key, );
+ if (!ret) {
+ req->dst_len = sizeof(digest);
I think you fixed the BUG_ON() problem but there's still an issue with the
handling of the digest. Check the use of sig->digest in
public_key_verify_signature(), it's an input not an output. Right now it
looks like 20 uninitialized bytes are compared with the computed digest
within verify_signature, and then the unintialized bytes are copied to
req->dst here.
With some modifications to public_key_verify_signature you could get the
digest you need, but I'm not sure if verification with a hardware key
(like a key in a TPM) can or can not provide the digest needed. Maybe this
is why the verify_signature hook in struct asymmetric_key_subtype is
optional.
+ scatterwalk_map_and_copy(digest, req->dst, 0, req->dst_len, 1);
+ }
+ kfree(src);
+ return ret;
+}
+
--
Mat Martineau
Intel OTC
Re: [PATCH v8 6/6] crypto: AF_ALG - add support for key_id
Tadeusz,
On Thu, 23 Jun 2016, Tadeusz Struk wrote:
This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options are
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_KEY_ID and ALG_SET_PUBKEY_ID for setting public and
private keys respectively. When these new options will be used
the user, instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.
When a key corresponding to the given keyid is found, it is stored
in the socket context and subsequent crypto operation invoked by the
user will use the new asymmetric accessor functions instead of akcipher
api. The asymmetric subtype can internally use akcipher api or
invoke operations defined by a given subtype, depending on the
key type.
Signed-off-by: Tadeusz Struk
---
crypto/af_alg.c | 10 ++
crypto/algif_akcipher.c | 212 ++-
include/crypto/if_alg.h |1
include/uapi/linux/if_alg.h |2
4 files changed, 220 insertions(+), 5 deletions(-)
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index 2b8d37e..106f715 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
+static int asym_key_verify(const struct key *key, struct akcipher_request *req)
+{
+ struct public_key_signature sig;
+ char *src = NULL, *in, digest[20];
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ sig.pkey_algo = "rsa";
+ sig.encoding = "pkcs1";
+ /* Need to find a way to pass the hash param */
Comment still needed?
+ sig.hash_algo = "sha1";
+ sig.digest_size = sizeof(digest);
+ sig.digest = digest;
+ sig.s_size = req->src_len;
+ sig.s = src;
+ ret = verify_signature(key, );
+ if (!ret) {
+ req->dst_len = sizeof(digest);
I think you fixed the BUG_ON() problem but there's still an issue with the
handling of the digest. Check the use of sig->digest in
public_key_verify_signature(), it's an input not an output. Right now it
looks like 20 uninitialized bytes are compared with the computed digest
within verify_signature, and then the unintialized bytes are copied to
req->dst here.
With some modifications to public_key_verify_signature you could get the
digest you need, but I'm not sure if verification with a hardware key
(like a key in a TPM) can or can not provide the digest needed. Maybe this
is why the verify_signature hook in struct asymmetric_key_subtype is
optional.
+ scatterwalk_map_and_copy(digest, req->dst, 0, req->dst_len, 1);
+ }
+ kfree(src);
+ return ret;
+}
+
--
Mat Martineau
Intel OTC
Re: [PATCH 5/8] KEYS: Provide software public key query function [ver #2]
David,
On Thu, 23 Jun 2016, David Howells wrote:
Provide a query function for the software public key implementation. This
permits information about such a key to be obtained using
query_asymmetric_key() or KEYCTL_PKEY_QUERY.
Signed-off-by: David Howells <dhowe...@redhat.com>
---
crypto/asymmetric_keys/public_key.c | 96 ++-
1 file changed, 82 insertions(+), 14 deletions(-)
diff --git a/crypto/asymmetric_keys/public_key.c
b/crypto/asymmetric_keys/public_key.c
index fd76b5fc3b3a..a48a47a1dff0 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -57,6 +57,81 @@ static void public_key_destroy(void *payload0, void
*payload3)
public_key_signature_free(payload3);
}
+/*
+ * Determine the crypto algorithm name.
+ */
+static
+int software_key_determine_akcipher(const char *encoding,
+ const char *hash_algo,
+ const struct public_key *pkey,
+ char alg_name[CRYPTO_MAX_ALG_NAME])
+{
+ int n;
+
+ if (strcmp(encoding, "pkcs1") == 0) {
+ /* The data wangled by the RSA algorithm is typically padded
+* and encoded in some manner, such as EMSA-PKCS1-1_5 [RFC3447
+* sec 8.2].
+*/
+ if (!hash_algo)
+ n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
+"pkcs1pad(%s)",
+pkey->pkey_algo);
Did you see Herbert's patch that strips out non-hash pkcs1pad capabilities
(and the ensuing discussion)?
http://www.spinics.net/lists/linux-crypto/index.html#20432
I'm making use of pkcs1pad(rsa) with a TLS implementation, so it's good to
see it supported here.
+ else
+ n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
+"pkcs1pad(%s,%s)",
+pkey->pkey_algo, hash_algo);
+ return n >= CRYPTO_MAX_ALG_NAME ? -EINVAL : 0;
+ }
+
+ if (strcmp(encoding, "raw") == 0) {
+ strcpy(alg_name, pkey->pkey_algo);
+ return 0;
+ }
+
+ return -ENOPKG;
+}
Regards,
--
Mat Martineau
Intel OTC
Re: [PATCH 5/8] KEYS: Provide software public key query function [ver #2]
David,
On Thu, 23 Jun 2016, David Howells wrote:
Provide a query function for the software public key implementation. This
permits information about such a key to be obtained using
query_asymmetric_key() or KEYCTL_PKEY_QUERY.
Signed-off-by: David Howells
---
crypto/asymmetric_keys/public_key.c | 96 ++-
1 file changed, 82 insertions(+), 14 deletions(-)
diff --git a/crypto/asymmetric_keys/public_key.c
b/crypto/asymmetric_keys/public_key.c
index fd76b5fc3b3a..a48a47a1dff0 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -57,6 +57,81 @@ static void public_key_destroy(void *payload0, void
*payload3)
public_key_signature_free(payload3);
}
+/*
+ * Determine the crypto algorithm name.
+ */
+static
+int software_key_determine_akcipher(const char *encoding,
+ const char *hash_algo,
+ const struct public_key *pkey,
+ char alg_name[CRYPTO_MAX_ALG_NAME])
+{
+ int n;
+
+ if (strcmp(encoding, "pkcs1") == 0) {
+ /* The data wangled by the RSA algorithm is typically padded
+* and encoded in some manner, such as EMSA-PKCS1-1_5 [RFC3447
+* sec 8.2].
+*/
+ if (!hash_algo)
+ n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
+"pkcs1pad(%s)",
+pkey->pkey_algo);
Did you see Herbert's patch that strips out non-hash pkcs1pad capabilities
(and the ensuing discussion)?
http://www.spinics.net/lists/linux-crypto/index.html#20432
I'm making use of pkcs1pad(rsa) with a TLS implementation, so it's good to
see it supported here.
+ else
+ n = snprintf(alg_name, CRYPTO_MAX_ALG_NAME,
+"pkcs1pad(%s,%s)",
+pkey->pkey_algo, hash_algo);
+ return n >= CRYPTO_MAX_ALG_NAME ? -EINVAL : 0;
+ }
+
+ if (strcmp(encoding, "raw") == 0) {
+ strcpy(alg_name, pkey->pkey_algo);
+ return 0;
+ }
+
+ return -ENOPKG;
+}
Regards,
--
Mat Martineau
Intel OTC
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
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
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Stephan,
On Sat, 14 May 2016, Tadeusz Struk wrote:
From: Stephan Mueller <smuel...@chronox.de>
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 <smuel...@chronox.de>
Signed-off-by: Tadeusz Struk <tadeusz.st...@intel.com>
---
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 = >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, );
+ 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
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 = >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, );
+ 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
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
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
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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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
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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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
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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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
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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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
Re: [PATCH v6 3/6] crypto: AF_ALG -- add asymmetric cipher interface
Stephan,
On Sat, 14 May 2016, Tadeusz Struk wrote:
From: Stephan Mueller <smuel...@chronox.de>
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 <smuel...@chronox.de>
Signed-off-by: Tadeusz Struk <tadeusz.st...@intel.com>
---
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 = >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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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(>req, sgl->sg, ctx->rsgl[0].sg, used,
+ usedpages);
+ switch (ctx->op) {
+ case ALG_OP_VERIFY:
+ err = crypto_akcipher_verify(>req);
+ break;
+ case ALG_OP_SIGN:
+ err = crypto_akcipher_sign(>req);
+ break;
+ case ALG_OP_ENCRYPT:
+ err = crypto_akcipher_encrypt(>req);
+ break;
+ case ALG_OP_DECRYPT:
+ err = crypto_akcipher_decrypt(>req);
+ break;
+ default:
+ err = -EFAULT;
+ goto unlock;
+ }
+
+ err = af_alg_wait_for_completion(err, >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(>rsgl[i]);
+
+ akcipher_wmem_wakeup(sk);
+ release_sock(sk);
+
+ return err ? err : ctx->req.dst_len;
+}
--
Mat Martineau
Intel OTC
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 = >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_iter)) {
+ /* make one iovec available as scatterlist */
+ err = af_alg_make_sg(>rsgl[cnt], >msg_iter,
+iov_iter_count(>msg_iter));
+ if (err < 0)
+ goto unlock;
+ usedpages += err;
+ /* chain the new scatterlist with previous one */
+ if (cnt)
+ af_alg_link_sg(>rsgl[cnt - 1], >rsgl[cnt]);
+
+ iov_iter_advance(>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(>req, sgl->sg, ctx->rsgl[0].sg, used,
+ usedpages);
+ switch (ctx->op) {
+ case ALG_OP_VERIFY:
+ err = crypto_akcipher_verify(>req);
+ break;
+ case ALG_OP_SIGN:
+ err = crypto_akcipher_sign(>req);
+ break;
+ case ALG_OP_ENCRYPT:
+ err = crypto_akcipher_encrypt(>req);
+ break;
+ case ALG_OP_DECRYPT:
+ err = crypto_akcipher_decrypt(>req);
+ break;
+ default:
+ err = -EFAULT;
+ goto unlock;
+ }
+
+ err = af_alg_wait_for_completion(err, >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(>rsgl[i]);
+
+ akcipher_wmem_wakeup(sk);
+ release_sock(sk);
+
+ return err ? err : ctx->req.dst_len;
+}
--
Mat Martineau
Intel OTC
Re: [PATCH v6 6/6] crypto: AF_ALG - add support for key_id
On Sat, 14 May 2016, Tadeusz Struk wrote:
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index e00793d..6733df1 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
+static int asym_key_verify(const struct key *key, struct akcipher_request *req)
+{
+ struct public_key_signature sig;
+ char *src = NULL, *in;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ sig.pkey_algo = "rsa";
+ sig.encoding = "pkcs1";
+ /* Need to find a way to pass the hash param */
Are you referring to sig.digest here? It looks like you will hit a
BUG_ON() in public_key_verify_signature() if sig.digest is 0. However,
sig.digest is unlikely to be 0 because the struct is not cleared - should
fix this, since public_key_verify_signature() will try to follow that
random pointer.
+ sig.hash_algo = "sha1";
+ sig.digest_size = 20;
+ sig.s_size = req->src_len;
+ sig.s = src;
+ ret = verify_signature(key, NULL, );
Is the idea to write the signature to the socket, and then read out the
expected digest (the digest comparison being done elsewhere)? Is that
something that will be supported by a future hardware asymmetric key
subtype?
verify_signature() ends up calling public_key_verify_signature(), which
currently expects to get both the digest and signature as input and
returns an error if verification fails. The output of
crypto_akcipher_verify() is discarded before public_key_verify_signature()
returns so nothing ends up in req->dst to read from the socket.
ALG_OP_VERIFY should behave the same whether using ALG_SET_PUBKEY or
ALG_SET_PUBKEY_ID, and they aren't right now.
If sig.digest is 0, verify_signature() could return the expected digest in
the sig structure and skip the digest comparison it currently does. Then
that data could be packaged up in req as if crypto_akcipher_verify() had
been called. I don't know if this change confuses the semantics of
verify_signature() too much, maybe a new function is required with all the
requisite plumbing to the asymmetric key subtype.
--
Mat Martineau
Intel OTC
Re: [PATCH v6 6/6] crypto: AF_ALG - add support for key_id
On Sat, 14 May 2016, Tadeusz Struk wrote:
diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c
index e00793d..6733df1 100644
--- a/crypto/algif_akcipher.c
+++ b/crypto/algif_akcipher.c
+static int asym_key_verify(const struct key *key, struct akcipher_request *req)
+{
+ struct public_key_signature sig;
+ char *src = NULL, *in;
+ int ret;
+
+ if (!sg_is_last(req->src)) {
+ src = kmalloc(req->src_len, GFP_KERNEL);
+ if (!src)
+ return -ENOMEM;
+ scatterwalk_map_and_copy(src, req->src, 0, req->src_len, 0);
+ in = src;
+ } else {
+ in = sg_virt(req->src);
+ }
+ sig.pkey_algo = "rsa";
+ sig.encoding = "pkcs1";
+ /* Need to find a way to pass the hash param */
Are you referring to sig.digest here? It looks like you will hit a
BUG_ON() in public_key_verify_signature() if sig.digest is 0. However,
sig.digest is unlikely to be 0 because the struct is not cleared - should
fix this, since public_key_verify_signature() will try to follow that
random pointer.
+ sig.hash_algo = "sha1";
+ sig.digest_size = 20;
+ sig.s_size = req->src_len;
+ sig.s = src;
+ ret = verify_signature(key, NULL, );
Is the idea to write the signature to the socket, and then read out the
expected digest (the digest comparison being done elsewhere)? Is that
something that will be supported by a future hardware asymmetric key
subtype?
verify_signature() ends up calling public_key_verify_signature(), which
currently expects to get both the digest and signature as input and
returns an error if verification fails. The output of
crypto_akcipher_verify() is discarded before public_key_verify_signature()
returns so nothing ends up in req->dst to read from the socket.
ALG_OP_VERIFY should behave the same whether using ALG_SET_PUBKEY or
ALG_SET_PUBKEY_ID, and they aren't right now.
If sig.digest is 0, verify_signature() could return the expected digest in
the sig structure and skip the digest comparison it currently does. Then
that data could be packaged up in req as if crypto_akcipher_verify() had
been called. I don't know if this change confuses the semantics of
verify_signature() too much, maybe a new function is required with all the
requisite plumbing to the asymmetric key subtype.
--
Mat Martineau
Intel OTC
Re: [PATCH RESEND v5 6/6] crypto: AF_ALG - add support for key_id
Tadeusz - David updated the keys-asym-keyctl branch, and this patch set won't build any more. On Thu, 5 May 2016, Tadeusz Struk wrote: diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c index e00793d..f486b6d 100644 --- a/crypto/algif_akcipher.c +++ b/crypto/algif_akcipher.c +static int asym_key_encrypt(const struct key *key, struct akcipher_request *req) ... + params.data_len = req->src_len; + params.enc_len = req->dst_len; The params member names have changed (now in_len and out_len). + ret = encrypt_blob(, in, out); The encrypt function for the key can now be called with params.key->type->asym_eds_op(). This also allows you to factor out the duplication in asym_key_encrypt, asym_key_decrypt, and asym_key_sign. See keyctl_pkey_e_d_s() in keyctl_pkey.c +static int asym_key_verify(const struct key *key, struct akcipher_request *req) ... + ret = verify_signature(key, NULL, ); key->type->asym_verify_signature() is available as well. Regards, -- Mat Martineau Intel OTC
Re: [PATCH RESEND v5 6/6] crypto: AF_ALG - add support for key_id
Tadeusz - David updated the keys-asym-keyctl branch, and this patch set won't build any more. On Thu, 5 May 2016, Tadeusz Struk wrote: diff --git a/crypto/algif_akcipher.c b/crypto/algif_akcipher.c index e00793d..f486b6d 100644 --- a/crypto/algif_akcipher.c +++ b/crypto/algif_akcipher.c +static int asym_key_encrypt(const struct key *key, struct akcipher_request *req) ... + params.data_len = req->src_len; + params.enc_len = req->dst_len; The params member names have changed (now in_len and out_len). + ret = encrypt_blob(, in, out); The encrypt function for the key can now be called with params.key->type->asym_eds_op(). This also allows you to factor out the duplication in asym_key_encrypt, asym_key_decrypt, and asym_key_sign. See keyctl_pkey_e_d_s() in keyctl_pkey.c +static int asym_key_verify(const struct key *key, struct akcipher_request *req) ... + ret = verify_signature(key, NULL, ); key->type->asym_verify_signature() is available as well. Regards, -- Mat Martineau Intel OTC
Re: [RFC PATCH 5/8] KEYS: Provide software public key query function [ver 3]
On Thu, 12 May 2016, David Howells wrote: Mat Martineau <mathew.j.martin...@linux.intel.com> wrote: + len = crypto_akcipher_maxsize(tfm); + info->key_size = len * 8; + info->max_data_size = len; + info->max_sig_size = len; + info->max_enc_size = len; + info->max_dec_size = len; If len > UINT16_MAX, should UINT16_MAX be reported as the max size? Similar question for len*8 and key_size. key_size is 32 bits, but the other sizes are all 16 bits, so you would need a 524288-bit key to exceed their capacity. I'm not sure that's likely anytime soon, but should I just make all the sizes 32-bit anyway? Given that cryto_akcipher_maxsize() returns an int and keyctl_pkey_query is part of the userspace API, I support bumping the sizes to 32-bit. -- Mat Martineau Intel OTC
Re: [RFC PATCH 5/8] KEYS: Provide software public key query function [ver 3]
On Thu, 12 May 2016, David Howells wrote: Mat Martineau wrote: + len = crypto_akcipher_maxsize(tfm); + info->key_size = len * 8; + info->max_data_size = len; + info->max_sig_size = len; + info->max_enc_size = len; + info->max_dec_size = len; If len > UINT16_MAX, should UINT16_MAX be reported as the max size? Similar question for len*8 and key_size. key_size is 32 bits, but the other sizes are all 16 bits, so you would need a 524288-bit key to exceed their capacity. I'm not sure that's likely anytime soon, but should I just make all the sizes 32-bit anyway? Given that cryto_akcipher_maxsize() returns an int and keyctl_pkey_query is part of the userspace API, I support bumping the sizes to 32-bit. -- Mat Martineau Intel OTC
Re: [RFC PATCH 8/8] KEYS: Implement PKCS#8 RSA Private Key parser [ver 3]
On Wed, 11 May 2016, David Howells wrote: diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile index 6516855bec18..417035a53e98 100644 --- a/crypto/asymmetric_keys/Makefile +++ b/crypto/asymmetric_keys/Makefile @@ -34,6 +34,19 @@ clean-files += x509_akid-asn1.c x509_akid-asn1.h # # PKCS#7 message handling Update to PKCS#8 # +obj-$(CONFIG_PKCS8_PRIVATE_KEY_PARSER) += pkcs8_key_parser.o +pkcs8_key_parser-y := \ + pkcs8-asn1.o \ + pkcs8_parser.o + +$(obj)/pkcs8_parser.o: $(obj)/pkcs8-asn1.h +$(obj)/pkcs8-asn1.o: $(obj)/pkcs8-asn1.c $(obj)/pkcs8-asn1.h + +clean-files+= pkcs8-asn1.c pkcs8-asn1.h -- Mat Martineau Intel OTC
Re: [RFC PATCH 8/8] KEYS: Implement PKCS#8 RSA Private Key parser [ver 3]
On Wed, 11 May 2016, David Howells wrote: diff --git a/crypto/asymmetric_keys/Makefile b/crypto/asymmetric_keys/Makefile index 6516855bec18..417035a53e98 100644 --- a/crypto/asymmetric_keys/Makefile +++ b/crypto/asymmetric_keys/Makefile @@ -34,6 +34,19 @@ clean-files += x509_akid-asn1.c x509_akid-asn1.h # # PKCS#7 message handling Update to PKCS#8 # +obj-$(CONFIG_PKCS8_PRIVATE_KEY_PARSER) += pkcs8_key_parser.o +pkcs8_key_parser-y := \ + pkcs8-asn1.o \ + pkcs8_parser.o + +$(obj)/pkcs8_parser.o: $(obj)/pkcs8-asn1.h +$(obj)/pkcs8-asn1.o: $(obj)/pkcs8-asn1.c $(obj)/pkcs8-asn1.h + +clean-files+= pkcs8-asn1.c pkcs8-asn1.h -- Mat Martineau Intel OTC
Re: [RFC PATCH 5/8] KEYS: Provide software public key query function [ver 3]
On Wed, 11 May 2016, David Howells wrote:
Provide a query function for the software public key implementation. This
permits information about such a key to be obtained using
query_asymmetric_key() or KEYCTL_PKEY_QUERY.
Signed-off-by: David Howells <dhowe...@redhat.com>
---
crypto/asymmetric_keys/public_key.c | 96 ++-
1 file changed, 82 insertions(+), 14 deletions(-)
diff --git a/crypto/asymmetric_keys/public_key.c
b/crypto/asymmetric_keys/public_key.c
index 96983906d2a2..e9967e5a2c25 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
+static int software_key_query(const struct kernel_pkey_params *params,
+ struct kernel_pkey_query *info)
+{
...
+ len = crypto_akcipher_maxsize(tfm);
+ info->key_size = len * 8;
+ info->max_data_size = len;
+ info->max_sig_size = len;
+ info->max_enc_size = len;
+ info->max_dec_size = len;
If len > UINT16_MAX, should UINT16_MAX be reported as the max size?
Similar question for len*8 and key_size.
--
Mat Martineau
Intel OTC
Re: [RFC PATCH 5/8] KEYS: Provide software public key query function [ver 3]
On Wed, 11 May 2016, David Howells wrote:
Provide a query function for the software public key implementation. This
permits information about such a key to be obtained using
query_asymmetric_key() or KEYCTL_PKEY_QUERY.
Signed-off-by: David Howells
---
crypto/asymmetric_keys/public_key.c | 96 ++-
1 file changed, 82 insertions(+), 14 deletions(-)
diff --git a/crypto/asymmetric_keys/public_key.c
b/crypto/asymmetric_keys/public_key.c
index 96983906d2a2..e9967e5a2c25 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
+static int software_key_query(const struct kernel_pkey_params *params,
+ struct kernel_pkey_query *info)
+{
...
+ len = crypto_akcipher_maxsize(tfm);
+ info->key_size = len * 8;
+ info->max_data_size = len;
+ info->max_sig_size = len;
+ info->max_enc_size = len;
+ info->max_dec_size = len;
If len > UINT16_MAX, should UINT16_MAX be reported as the max size?
Similar question for len*8 and key_size.
--
Mat Martineau
Intel OTC
Re: [RFC PATCH 2/8] KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver 3]
On Wed, 11 May 2016, David Howells wrote: diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt index ca72b70a24b9..01c2ae28a8c0 100644 --- a/Documentation/security/keys.txt +++ b/Documentation/security/keys.txt + If the key needs to be unlocked with a password, a logon-type key that + holds the password may be given as the password argument ... + If the key must be unlocked with a password before it can be used, + password_id should point to a logon-type key that holds this. It should be noted that the password_id should be 0 if no password is to be used. diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c new file mode 100644 index ..7f51db984aaa --- /dev/null +++ b/security/keys/keyctl_pkey.c +long keyctl_pkey_e_d_s(int op, ... + ret = params.key->type->asym_eds_op(, in, out); Need to check for NULL asym_eds_op before calling. Regards, -- Mat Martineau Intel OTC
Re: [RFC PATCH 2/8] KEYS: Provide keyctls to drive the new key type ops for asymmetric keys [ver 3]
On Wed, 11 May 2016, David Howells wrote: diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt index ca72b70a24b9..01c2ae28a8c0 100644 --- a/Documentation/security/keys.txt +++ b/Documentation/security/keys.txt + If the key needs to be unlocked with a password, a logon-type key that + holds the password may be given as the password argument ... + If the key must be unlocked with a password before it can be used, + password_id should point to a logon-type key that holds this. It should be noted that the password_id should be 0 if no password is to be used. diff --git a/security/keys/keyctl_pkey.c b/security/keys/keyctl_pkey.c new file mode 100644 index ..7f51db984aaa --- /dev/null +++ b/security/keys/keyctl_pkey.c +long keyctl_pkey_e_d_s(int op, ... + ret = params.key->type->asym_eds_op(, in, out); Need to check for NULL asym_eds_op before calling. Regards, -- Mat Martineau Intel OTC
Re: [RFC PATCH] KEYS: Provide keyctls to do public key operations
Opt_enc,/* "enc=" eg. "enc=oaep" */ endoding->encoding + Opt_hash, /* "hash=" eg. "hash=sha1" */ +}; -- Mat Martineau Intel OTC
Re: [RFC PATCH] KEYS: Provide keyctls to do public key operations
Opt_enc,/* "enc=" eg. "enc=oaep" */ endoding->encoding + Opt_hash, /* "hash=" eg. "hash=sha1" */ +}; -- Mat Martineau Intel OTC
Re: [RFC PATCH 02/12] PKCS#7: Make trust determination dependent on contents of trust keyring [ver #4]
On Thu, 7 Apr 2016, David Howells wrote:
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c
b/crypto/asymmetric_keys/pkcs7_trust.c
index b9a5487cd82d..0dccb6fe7634 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -170,8 +162,7 @@ verified:
* May also return -ENOMEM.
*/
int pkcs7_validate_trust(struct pkcs7_message *pkcs7,
-struct key *trust_keyring,
-bool *_trusted)
This doesn't build after the keys-trust branch was rebased on v4.6-rc2. A
reference to _trusted was added in commit e5435891 ("PKCS#7:
pkcs7_validate_trust(): initialize the _trusted output argument"), right
after the local declarations.
+struct key *trust_keyring)
{
struct pkcs7_signed_info *sinfo;
struct x509_certificate *p;
Regards,
--
Mat Martineau
Intel OTC
Re: [RFC PATCH 02/12] PKCS#7: Make trust determination dependent on contents of trust keyring [ver #4]
On Thu, 7 Apr 2016, David Howells wrote:
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c
b/crypto/asymmetric_keys/pkcs7_trust.c
index b9a5487cd82d..0dccb6fe7634 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -170,8 +162,7 @@ verified:
* May also return -ENOMEM.
*/
int pkcs7_validate_trust(struct pkcs7_message *pkcs7,
-struct key *trust_keyring,
-bool *_trusted)
This doesn't build after the keys-trust branch was rebased on v4.6-rc2. A
reference to _trusted was added in commit e5435891 ("PKCS#7:
pkcs7_validate_trust(): initialize the _trusted output argument"), right
after the local declarations.
+struct key *trust_keyring)
{
struct pkcs7_signed_info *sinfo;
struct x509_certificate *p;
Regards,
--
Mat Martineau
Intel OTC
