Netdev List
 help / color / mirror / Atom feed
* KASAN: global-out-of-bounds Read in validate_nla
From: syzbot @ 2019-01-24 18:23 UTC (permalink / raw)
  To: davem, johannes, linux-kernel, linux-wireless, netdev,
	syzkaller-bugs

Hello,

syzbot found the following crash on:

HEAD commit:    30bac164aca7 Revert "Change mincore() to count "mapped" pa..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17b824c0c00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=505743eba4e4f68
dashboard link: https://syzkaller.appspot.com/bug?extid=4157b036c5f4713b1f2f
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1137adcf400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16431b18c00000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+4157b036c5f4713b1f2f@syzkaller.appspotmail.com

audit: type=1800 audit(1548293866.971:30): pid=7616 uid=0 auid=4294967295  
ses=4294967295 subj==unconfined op=collect_data cause=failed(directio)  
comm="startpar" name="rmnologin" dev="sda1" ino=2423 res=0
==================================================================
BUG: KASAN: global-out-of-bounds in validate_nla+0x12c4/0x1580  
lib/nlattr.c:169
Read of size 1 at addr ffffffff88f41fc0 by task syz-executor771/7789

CPU: 1 PID: 7789 Comm: syz-executor771 Not tainted 5.0.0-rc3+ #40
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x1db/0x2d0 lib/dump_stack.c:113
  print_address_description.cold+0x5/0x20d mm/kasan/report.c:187
  kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
  __asan_report_load1_noabort+0x14/0x20 mm/kasan/generic_report.c:132
  validate_nla+0x12c4/0x1580 lib/nlattr.c:169
  nla_validate+0xc1/0x130 lib/nlattr.c:340
  validate_nla+0x711/0x1580 lib/nlattr.c:247
  __nla_parse+0x206/0x340 lib/nlattr.c:415
  nla_parse+0x45/0x60 lib/nlattr.c:439
  nlmsg_parse include/net/netlink.h:527 [inline]
  nl80211_dump_wiphy_parse.isra.0.constprop.0+0x133/0x610  
net/wireless/nl80211.c:2299
  nl80211_dump_wiphy+0x595/0x760 net/wireless/nl80211.c:2342
  genl_lock_dumpit+0x6d/0xa0 net/netlink/genetlink.c:481
  netlink_dump+0x5f2/0x1070 net/netlink/af_netlink.c:2244
  __netlink_dump_start+0x5b4/0x7e0 net/netlink/af_netlink.c:2352
  genl_family_rcv_msg+0xeb5/0x11a0 net/netlink/genetlink.c:549
  genl_rcv_msg+0xca/0x16c net/netlink/genetlink.c:626
  netlink_rcv_skb+0x17d/0x410 net/netlink/af_netlink.c:2477
  genl_rcv+0x29/0x40 net/netlink/genetlink.c:637
  netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
  netlink_unicast+0x574/0x770 net/netlink/af_netlink.c:1336
  netlink_sendmsg+0xa05/0xf90 net/netlink/af_netlink.c:1917
  sock_sendmsg_nosec net/socket.c:621 [inline]
  sock_sendmsg+0xdd/0x130 net/socket.c:631
  ___sys_sendmsg+0x7ec/0x910 net/socket.c:2116
  __sys_sendmsg+0x112/0x270 net/socket.c:2154
  __do_sys_sendmsg net/socket.c:2163 [inline]
  __se_sys_sendmsg net/socket.c:2161 [inline]
  __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2161
  do_syscall_64+0x1a3/0x800 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x4400d9
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 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 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffd4f4bf478 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004400d9
RDX: 0000000000000000 RSI: 0000000020000380 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000006 R09: 00000000004002c8
R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000401960
R13: 00000000004019f0 R14: 0000000000000000 R15: 0000000000000000

The buggy address belongs to the variable:
  nl80211_pmsr_attr_policy+0x60/0x80

Memory state around the buggy address:
  ffffffff88f41e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  ffffffff88f41f00: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
> ffffffff88f41f80: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
                                            ^
  ffffffff88f42000: 00 00 00 00 00 00 fa fa fa fa fa fa 00 00 00 00
  ffffffff88f42080: 00 00 fa fa fa fa fa fa 00 00 00 00 fa fa fa fa
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply

* [PATCH v2] mac80211: Add attribute aligned(2) to struct 'action'
From: Mathieu Malaterre @ 2019-01-24 18:19 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Mathieu Malaterre, Johannes Berg, David S. Miller, linux-wireless,
	netdev, linux-kernel
In-Reply-To: <20190124180535.20216-1-malat@debian.org>

During refactor in commit 9e478066eae4 ("mac80211: fix MU-MIMO
follow-MAC mode") a new struct 'action' was declared with packed
attribute as:

  struct {
          struct ieee80211_hdr_3addr hdr;
          u8 category;
          u8 action_code;
  } __packed action;

But since struct 'ieee80211_hdr_3addr' is declared with an aligned
keyword as:

  struct ieee80211_hdr {
  	__le16 frame_control;
  	__le16 duration_id;
  	u8 addr1[ETH_ALEN];
  	u8 addr2[ETH_ALEN];
  	u8 addr3[ETH_ALEN];
  	__le16 seq_ctrl;
  	u8 addr4[ETH_ALEN];
  } __packed __aligned(2);

Solve the ambiguity of placing aligned structure in a packed one by
adding the aligned(2) attribute to struct 'action'. This seems to be the
behavior of gcc anyway, since the following is still compiling:

  BUILD_BUG_ON(sizeof(action) != IEEE80211_MIN_ACTION_SIZE + 1);

This removes the following warning (W=1):

  net/mac80211/rx.c:234:2: warning: alignment 1 of 'struct <anonymous>' is less than 2 [-Wpacked-not-aligned]

Cc: Johannes Berg <johannes.berg@intel.com>
Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
v2: It was suggested by Johannes that an arch actually need the pack attribute (alpha).

 net/mac80211/rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 45aad3d3108c..885df250b67e 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -231,7 +231,7 @@ static void ieee80211_handle_mu_mimo_mon(struct ieee80211_sub_if_data *sdata,
 		struct ieee80211_hdr_3addr hdr;
 		u8 category;
 		u8 action_code;
-	} __packed action;
+	} __packed __aligned(2) action;
 
 	if (!sdata)
 		return;
-- 
2.19.2


^ permalink raw reply related

* Re: [PATCH] mac80211: Remove attribute packed from struct 'action'
From: Johannes Berg @ 2019-01-24 18:19 UTC (permalink / raw)
  To: Mathieu Malaterre; +Cc: David S. Miller, linux-wireless, netdev, LKML
In-Reply-To: <CA+7wUszXbRt=j1C1PF11EtnO2ojLpvL8Zs60c7UcEDrqjRMxOw@mail.gmail.com>

On Thu, 2019-01-24 at 19:14 +0100, Mathieu Malaterre wrote:

> > I'm not sure this will work on all platforms, didn't something like
> > alpha pad out u8's to u32 when not requiring packing?
> 
> I was not aware of that.

TBH, I'm not actually sure about that. Pure hearsay. If anyone knows,
I'd like to know too :)

johannes


^ permalink raw reply

* Re: [PATCH] mac80211: Remove attribute packed from struct 'action'
From: Mathieu Malaterre @ 2019-01-24 18:14 UTC (permalink / raw)
  To: Johannes Berg; +Cc: David S. Miller, linux-wireless, netdev, LKML
In-Reply-To: <2b8b377a84883db1a45afb42204d8fb5a7d5ccf9.camel@sipsolutions.net>

On Thu, Jan 24, 2019 at 7:08 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Thu, 2019-01-24 at 19:05 +0100, Mathieu Malaterre wrote:
> > During refactor in commit 9e478066eae4 ("mac80211: fix MU-MIMO
> > follow-MAC mode") a new struct 'action' was declared with packed
> > attribute as:
> >
> >   struct {
> >           struct ieee80211_hdr_3addr hdr;
> >           u8 category;
> >           u8 action_code;
> >   } __packed action;
> >
> > But since struct 'ieee80211_hdr_3addr' is declared with an aligned
> > keyword as:
> >
> >   struct ieee80211_hdr {
> >       __le16 frame_control;
> >       __le16 duration_id;
> >       u8 addr1[ETH_ALEN];
> >       u8 addr2[ETH_ALEN];
> >       u8 addr3[ETH_ALEN];
> >       __le16 seq_ctrl;
> >       u8 addr4[ETH_ALEN];
> >   } __packed __aligned(2);
> >
> > Solve the ambiguity of placing aligned structure in a packed one by
> > removing the packed attribute from struct. This seems to be the behavior
> > of gcc anyway, since the following is still compiling:
> >
> >   BUILD_BUG_ON(sizeof(action) != IEEE80211_MIN_ACTION_SIZE + 1);
>
> I'm not sure this will work on all platforms, didn't something like
> alpha pad out u8's to u32 when not requiring packing?

I was not aware of that.

> I guess I'd feel better about using __packed __aligned(2) here as well,
> which should solve the warning too?

Indeed, I will re-spin a v2 then.

^ permalink raw reply

* Re: [PATCH bpf-next v2] selftests/bpf: suppress readelf stderr when probing for BTF support
From: Song Liu @ 2019-01-24 18:14 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, David S . Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190124165429.36378-1-sdf@google.com>

On Thu, Jan 24, 2019 at 8:57 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Before:
> $ make -s -C tools/testing/selftests/bpf
> readelf: Error: Missing knowledge of 32-bit reloc types used in DWARF
> sections of machine number 247
> readelf: Warning: unable to apply unsupported reloc type 10 to section
> .debug_info
> readelf: Warning: unable to apply unsupported reloc type 1 to section
> .debug_info
> readelf: Warning: unable to apply unsupported reloc type 10 to section
> .debug_info
>
> After:
> $ make -s -C tools/testing/selftests/bpf
>
> v2:
> * use llvm-readelf instead of redirecting binutils' readelf stderr to
>   /dev/null
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

This looks good.

Acked-by: Song Liu <songliubraving@fb.com>

Thanks!

> ---
>  tools/testing/selftests/bpf/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 70229de510f5..1cca5f2567c2 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -95,6 +95,7 @@ $(BPFOBJ): force
>  CLANG ?= clang
>  LLC   ?= llc
>  LLVM_OBJCOPY ?= llvm-objcopy
> +LLVM_READELF ?= llvm-readelf
>  BTF_PAHOLE ?= pahole
>
>  PROBE := $(shell $(LLC) -march=bpf -mcpu=probe -filetype=null /dev/null 2>&1)
> @@ -131,7 +132,7 @@ BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
>  BTF_OBJCOPY_PROBE := $(shell $(LLVM_OBJCOPY) --help 2>&1 | grep -i 'usage.*llvm')
>  BTF_LLVM_PROBE := $(shell echo "int main() { return 0; }" | \
>                           $(CLANG) -target bpf -O2 -g -c -x c - -o ./llvm_btf_verify.o; \
> -                         readelf -S ./llvm_btf_verify.o | grep BTF; \
> +                         $(LLVM_READELF) -S ./llvm_btf_verify.o | grep BTF; \
>                           /bin/rm -f ./llvm_btf_verify.o)
>
>  ifneq ($(BTF_LLVM_PROBE),)
> --
> 2.20.1.321.g9e740568ce-goog
>

^ permalink raw reply

* Re: [PATCH net-next v1 1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X
From: Andrew Lunn @ 2019-01-24 18:11 UTC (permalink / raw)
  To: Marek Behun; +Cc: netdev, Florian Fainelli, David Miller
In-Reply-To: <20190124190451.40af3127@nic.cz>

On Thu, Jan 24, 2019 at 07:04:51PM +0100, Marek Behun wrote:
> What properties does the cpu port node need to contain to force it?
> phy-mode = "2500base-x"; is not enough.

Hi Marek

For DSA ports we have:

                                                phy-mode = "rgmii-txid";
                                                fixed-link {
                                                        speed = <1000>;
                                                        full-duplex;
                                                };

See dsa_port_fixed_link_register_of()

    Andrew

^ permalink raw reply

* Re: [PATCH] mac80211: Remove attribute packed from struct 'action'
From: Johannes Berg @ 2019-01-24 18:08 UTC (permalink / raw)
  To: Mathieu Malaterre; +Cc: David S. Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190124180535.20216-1-malat@debian.org>

On Thu, 2019-01-24 at 19:05 +0100, Mathieu Malaterre wrote:
> During refactor in commit 9e478066eae4 ("mac80211: fix MU-MIMO
> follow-MAC mode") a new struct 'action' was declared with packed
> attribute as:
> 
>   struct {
>           struct ieee80211_hdr_3addr hdr;
>           u8 category;
>           u8 action_code;
>   } __packed action;
> 
> But since struct 'ieee80211_hdr_3addr' is declared with an aligned
> keyword as:
> 
>   struct ieee80211_hdr {
>   	__le16 frame_control;
>   	__le16 duration_id;
>   	u8 addr1[ETH_ALEN];
>   	u8 addr2[ETH_ALEN];
>   	u8 addr3[ETH_ALEN];
>   	__le16 seq_ctrl;
>   	u8 addr4[ETH_ALEN];
>   } __packed __aligned(2);
> 
> Solve the ambiguity of placing aligned structure in a packed one by
> removing the packed attribute from struct. This seems to be the behavior
> of gcc anyway, since the following is still compiling:
> 
>   BUILD_BUG_ON(sizeof(action) != IEEE80211_MIN_ACTION_SIZE + 1);

I'm not sure this will work on all platforms, didn't something like
alpha pad out u8's to u32 when not requiring packing?

I guess I'd feel better about using __packed __aligned(2) here as well,
which should solve the warning too?

johannes



^ permalink raw reply

* [PATCH 1/2] Bluetooth: Annotate implicit fall through in l2cap_config_rsp
From: Mathieu Malaterre @ 2019-01-24 18:07 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Mathieu Malaterre, Gustavo Padovan, Johan Hedberg,
	David S. Miller, linux-bluetooth, netdev, linux-kernel

There is a plan to build the kernel with -Wimplicit-fallthrough and
this place in the code produced a warning (W=1).

This commit removes the following warning:

  net/bluetooth/l2cap_core.c:4223:6: warning: this statement may fall through [-Wimplicit-fallthrough=]

Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
 net/bluetooth/l2cap_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2a7fb517d460..a5b76e24940a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4244,6 +4244,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
 				goto done;
 			break;
 		}
+		/* fall through */
 
 	default:
 		l2cap_chan_set_err(chan, ECONNRESET);
-- 
2.19.2


^ permalink raw reply related

* [PATCH 2/2] Bluetooth: Correctly annotate implicit fall through in __rfcomm_dlc_close
From: Mathieu Malaterre @ 2019-01-24 18:07 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Mathieu Malaterre, Peter Hurley, Johan Hedberg, David S. Miller,
	linux-bluetooth, netdev, linux-kernel
In-Reply-To: <20190124180724.20910-1-malat@debian.org>

There is a plan to build the kernel with -Wimplicit-fallthrough and
this place in the code produced a warning (W=1).

In this particular case put the fall through comment on a separate line so
as to match the regular expression expected by GCC.

This commit removes the following warning:

  net/bluetooth/rfcomm/core.c:479:6: warning: this statement may fall through [-Wimplicit-fallthrough=]

Cc: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
 net/bluetooth/rfcomm/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 1a635df80643..caaae6accafa 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -480,9 +480,8 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
 			__rfcomm_dlc_disconn(d);
 			break;
 		}
-		/* if closing a dlc in a session that hasn't been started,
-		 * just close and unlink the dlc
-		 */
+		/* fall through - if closing a dlc in a session that hasn't */
+		/* been started, just close and unlink the dlc */
 
 	default:
 		rfcomm_dlc_clear_timer(d);
-- 
2.19.2


^ permalink raw reply related

* [PATCH] mac80211: Remove attribute packed from struct 'action'
From: Mathieu Malaterre @ 2019-01-24 18:05 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Mathieu Malaterre, Johannes Berg, David S. Miller, linux-wireless,
	netdev, linux-kernel

During refactor in commit 9e478066eae4 ("mac80211: fix MU-MIMO
follow-MAC mode") a new struct 'action' was declared with packed
attribute as:

  struct {
          struct ieee80211_hdr_3addr hdr;
          u8 category;
          u8 action_code;
  } __packed action;

But since struct 'ieee80211_hdr_3addr' is declared with an aligned
keyword as:

  struct ieee80211_hdr {
  	__le16 frame_control;
  	__le16 duration_id;
  	u8 addr1[ETH_ALEN];
  	u8 addr2[ETH_ALEN];
  	u8 addr3[ETH_ALEN];
  	__le16 seq_ctrl;
  	u8 addr4[ETH_ALEN];
  } __packed __aligned(2);

Solve the ambiguity of placing aligned structure in a packed one by
removing the packed attribute from struct. This seems to be the behavior
of gcc anyway, since the following is still compiling:

  BUILD_BUG_ON(sizeof(action) != IEEE80211_MIN_ACTION_SIZE + 1);

This removes the following warning (W=1):

  net/mac80211/rx.c:234:2: warning: alignment 1 of 'struct <anonymous>' is less than 2 [-Wpacked-not-aligned]

Cc: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
 net/mac80211/rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 45aad3d3108c..709359650149 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -231,7 +231,7 @@ static void ieee80211_handle_mu_mimo_mon(struct ieee80211_sub_if_data *sdata,
 		struct ieee80211_hdr_3addr hdr;
 		u8 category;
 		u8 action_code;
-	} __packed action;
+	} action;
 
 	if (!sdata)
 		return;
-- 
2.19.2


^ permalink raw reply related

* Re: [PATCH net-next v1 1/2] net: dsa: mv88e6xxx: Default CMODE to 1000BaseX only on 6390X
From: Marek Behun @ 2019-01-24 18:04 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: netdev, Florian Fainelli, David Miller
In-Reply-To: <20190124164805.GJ482@lunn.ch>

What properties does the cpu port node need to contain to force it?
phy-mode = "2500base-x"; is not enough.
Marek

On Thu, 24 Jan 2019 17:48:05 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > yes, I wanted to set the cpu port mode in DTS at first, but if I
> > understood the code correctly (maybe I made a mistake), the CPU port
> > mode is not determined from DTS (since there is no phy). Am I
> > wrong?  
> 
> It should work. I have boards which use it for DSA ports. I'm not sure
> i have a board with it for a CPU port though.
> 
>    Andrew


^ permalink raw reply

* Re: [PATCH v4 bpf-next 1/9] bpf: introduce bpf_spin_lock
From: Peter Zijlstra @ 2019-01-24 18:01 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: davem, daniel, jakub.kicinski, netdev, kernel-team, mingo,
	will.deacon, Paul McKenney, jannh
In-Reply-To: <20190124041403.2100609-2-ast@kernel.org>


Thanks for having kernel/locking people on Cc...

On Wed, Jan 23, 2019 at 08:13:55PM -0800, Alexei Starovoitov wrote:

> Implementation details:
> - on !SMP bpf_spin_lock() becomes nop

Because no BPF program is preemptible? I don't see any assertions or
even a comment that says this code is non-preemptible.

AFAICT some of the BPF_RUN_PROG things are under rcu_read_lock() only,
which is not sufficient.

> - on architectures that don't support queued_spin_lock trivial lock is used.
>   Note that arch_spin_lock cannot be used, since not all archs agree that
>   zero == unlocked and sizeof(arch_spinlock_t) != sizeof(__u32).

I really don't much like direct usage of qspinlock; esp. not as a
surprise.

Why does it matter if 0 means unlocked; that's what
__ARCH_SPIN_LOCK_UNLOCKED is for.

I get the sizeof(__u32) thing, but why not key off of that?

> Next steps:
> - allow bpf_spin_lock in other map types (like cgroup local storage)
> - introduce BPF_F_LOCK flag for bpf_map_update() syscall and helper
>   to request kernel to grab bpf_spin_lock before rewriting the value.
>   That will serialize access to map elements.

So clearly this map stuff is shared between bpf proglets, otherwise
there would not be a need for locking. But what happens if one is from
task context and another from IRQ context?

I don't see a local_irq_save()/restore() anywhere. What avoids the
trivial lock inversion?


> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a74972b07e74..2e98e4caf5aa 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -221,6 +221,63 @@ const struct bpf_func_proto bpf_get_current_comm_proto = {
>  	.arg2_type	= ARG_CONST_SIZE,
>  };
>  
> +#ifndef CONFIG_QUEUED_SPINLOCKS
> +struct dumb_spin_lock {
> +	atomic_t val;
> +};
> +#endif
> +
> +notrace BPF_CALL_1(bpf_spin_lock, struct bpf_spin_lock *, lock)
> +{
> +#if defined(CONFIG_SMP)
> +#ifdef CONFIG_QUEUED_SPINLOCKS
> +	struct qspinlock *qlock = (void *)lock;
> +
> +	BUILD_BUG_ON(sizeof(*qlock) != sizeof(*lock));
> +	queued_spin_lock(qlock);
> +#else
> +	struct dumb_spin_lock *qlock = (void *)lock;
> +
> +	BUILD_BUG_ON(sizeof(*qlock) != sizeof(*lock));
> +	do {
> +		while (atomic_read(&qlock->val) != 0)
> +			cpu_relax();
> +	} while (atomic_cmpxchg(&qlock->val, 0, 1) != 0);
> +#endif
> +#endif
> +	return 0;
> +}
> +
> +const struct bpf_func_proto bpf_spin_lock_proto = {
> +	.func		= bpf_spin_lock,
> +	.gpl_only	= false,
> +	.ret_type	= RET_VOID,
> +	.arg1_type	= ARG_PTR_TO_SPIN_LOCK,
> +};
> +
> +notrace BPF_CALL_1(bpf_spin_unlock, struct bpf_spin_lock *, lock)
> +{
> +#if defined(CONFIG_SMP)
> +#ifdef CONFIG_QUEUED_SPINLOCKS
> +	struct qspinlock *qlock = (void *)lock;
> +
> +	queued_spin_unlock(qlock);
> +#else
> +	struct dumb_spin_lock *qlock = (void *)lock;
> +
> +	atomic_set(&qlock->val, 0);

And this is broken... That should've been atomic_set_release() at the
very least.

And this would again be the moment where I go pester you about the BPF
memory model :-)

> +#endif
> +#endif
> +	return 0;
> +}
> +
> +const struct bpf_func_proto bpf_spin_unlock_proto = {
> +	.func		= bpf_spin_unlock,
> +	.gpl_only	= false,
> +	.ret_type	= RET_VOID,
> +	.arg1_type	= ARG_PTR_TO_SPIN_LOCK,
> +};

^ permalink raw reply

* Re: [PATCH stable 4.4 05/11] ip: use rb trees for IP frag queue.
From: Greg KH @ 2019-01-24 17:58 UTC (permalink / raw)
  To: Mao Wenan; +Cc: netdev, eric.dumazet, davem, stable, edumazet
In-Reply-To: <1548209986-83527-6-git-send-email-maowenan@huawei.com>

On Wed, Jan 23, 2019 at 10:19:40AM +0800, Mao Wenan wrote:
> From: Peter Oskolkov <posk@google.com>
> 
> [ Upstream commit fa0f527358bd900ef92f925878ed6bfbd51305cc ]

This commit is not in the 4.14.y tree, any specific reason why not?

thanks,

greg k-h

^ permalink raw reply

* Re: [PATCH] NET:AX25:ROSE NULL ax25_cb kernel panic
From: Bernard Pidoux @ 2019-01-24 17:30 UTC (permalink / raw)
  To: linux-hams
  Cc: Ralf Baechle, Dmitry Vyukov, David Miller, Linux Netdev List,
	n1uro, f6bvp, Dmitry Vyukov
In-Reply-To: <19a0a879-8a8d-4c84-5de9-5ce222377fcb@free.fr>

> [PATCH] [ROSE] NULL ax25_cb kernel panic
>
> When an internally generated frame is handled by rose_xmit(),
> rose_route_frame() is called:
>
>          if (!rose_route_frame(skb, NULL)) {
>                  dev_kfree_skb(skb);
>                  stats->tx_errors++;
>                  return NETDEV_TX_OK;
>          }
>
> We have the same code sequence in Net/Rom where an internally generated
> frame is handled by nr_xmit() calling nr_route_frame(skb, NULL).
> However, in this function NULL argument is tested while it is not in
> rose_route_frame().
> Then kernel panic occurs later on when calling ax25cmp() with a NULL
> ax25_cb argument as reported many times and recently with syzbot.
>
> We need to test if ax25 is NULL before using it.
>
> Here is the patch:
>
> diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
> index 77e9f85a2c92..7f075255a372 100644
> --- a/net/rose/rose_route.c
> +++ b/net/rose/rose_route.c
> @@ -850,6 +850,7 @@ void rose_link_device_down(struct net_device *dev)
>
>   /*
>    *	Route a frame to an appropriate AX.25 connection.
> + *	a NULL ax25_cb indicates an internally generated frame.
>    */
>   int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)
>   {
> @@ -867,6 +868,10 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb
> *ax25)
>
>   	if (skb->len < ROSE_MIN_LEN)
>   		return res;
> +
> +	if (!ax25)
> +		return rose_loopback_queue(skb, NULL);
> +
>   	frametype = skb->data[2];
>   	lci = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF);
>   	if (frametype == ROSE_CALL_REQUEST &&
> Reported-by:syzbot+1a2c456a1ea08fa5b5f7@syzkaller.appspotmail.com
> Signed-off-by: Bernard Pidoux, f6bvp <f6bvp@free.fr>


^ permalink raw reply

* [PATCH] Bluetooth: make hw_err static, reduces object code size
From: Colin King @ 2019-01-24 17:22 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, David S . Miller, linux-bluetooth,
	netdev
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

Don't populate the const array hw_err on the stack but instead make
it static. Makes the object code smaller by 45 bytes:

Before:
   text	   data	    bss	    dec	    hex	filename
 100880	  21090	   1088	 123058	  1e0b2	linux/net/bluetooth/hci_core.o

After:
   text	   data	    bss	    dec	    hex	filename
 100739	  21186	   1088	 123013	  1e085	linux/net/bluetooth/hci_core.o

(gcc version 8.2.0 x86_64)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 net/bluetooth/hci_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..40cd6b48473a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3401,7 +3401,7 @@ EXPORT_SYMBOL(hci_resume_dev);
 /* Reset HCI device */
 int hci_reset_dev(struct hci_dev *hdev)
 {
-	const u8 hw_err[] = { HCI_EV_HARDWARE_ERROR, 0x01, 0x00 };
+	static const u8 hw_err[] = { HCI_EV_HARDWARE_ERROR, 0x01, 0x00 };
 	struct sk_buff *skb;
 
 	skb = bt_skb_alloc(3, GFP_ATOMIC);
-- 
2.19.1


^ permalink raw reply related

* [PATCH net-next 7/7] net: tls: Add tests for TLS 1.3
From: Dave Watson @ 2019-01-24 17:21 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

Change most tests to TLS 1.3, while adding tests for previous TLS 1.2
behavior.

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 tools/testing/selftests/net/tls.c | 76 ++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index c356f481de79..4ac50ccb3272 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -42,7 +42,7 @@ FIXTURE_SETUP(tls)
 	len = sizeof(addr);
 
 	memset(&tls12, 0, sizeof(tls12));
-	tls12.info.version = TLS_1_2_VERSION;
+	tls12.info.version = TLS_1_3_VERSION;
 	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
 
 	addr.sin_family = AF_INET;
@@ -825,4 +825,78 @@ TEST(keysizes) {
 	close(cfd);
 }
 
+TEST(tls12) {
+	int fd, cfd;
+	bool notls;
+
+	struct tls12_crypto_info_aes_gcm_128 tls12;
+	struct sockaddr_in addr;
+	socklen_t len;
+	int sfd, ret;
+
+	notls = false;
+	len = sizeof(addr);
+
+	memset(&tls12, 0, sizeof(tls12));
+	tls12.info.version = TLS_1_2_VERSION;
+	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
+
+	addr.sin_family = AF_INET;
+	addr.sin_addr.s_addr = htonl(INADDR_ANY);
+	addr.sin_port = 0;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	sfd = socket(AF_INET, SOCK_STREAM, 0);
+
+	ret = bind(sfd, &addr, sizeof(addr));
+	ASSERT_EQ(ret, 0);
+	ret = listen(sfd, 10);
+	ASSERT_EQ(ret, 0);
+
+	ret = getsockname(sfd, &addr, &len);
+	ASSERT_EQ(ret, 0);
+
+	ret = connect(fd, &addr, sizeof(addr));
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
+	if (ret != 0) {
+		notls = true;
+		printf("Failure setting TCP_ULP, testing without tls\n");
+	}
+
+	if (!notls) {
+		ret = setsockopt(fd, SOL_TLS, TLS_TX, &tls12,
+				 sizeof(tls12));
+		ASSERT_EQ(ret, 0);
+	}
+
+	cfd = accept(sfd, &addr, &len);
+	ASSERT_GE(cfd, 0);
+
+	if (!notls) {
+		ret = setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls",
+				 sizeof("tls"));
+		ASSERT_EQ(ret, 0);
+
+		ret = setsockopt(cfd, SOL_TLS, TLS_RX, &tls12,
+				 sizeof(tls12));
+		ASSERT_EQ(ret, 0);
+	}
+
+	close(sfd);
+
+	char const *test_str = "test_read";
+	int send_len = 10;
+	char buf[10];
+
+	send_len = strlen(test_str) + 1;
+	EXPECT_EQ(send(fd, test_str, send_len, 0), send_len);
+	EXPECT_NE(recv(cfd, buf, send_len, 0), -1);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+
+	close(fd);
+	close(cfd);
+}
+
 TEST_HARNESS_MAIN
-- 
2.17.1


^ permalink raw reply related

* [PATCH net-next 5/7] net: tls: Refactor control message handling on recv
From: Dave Watson @ 2019-01-24 17:21 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

For TLS 1.3, the control message is encrypted.  Handle control
message checks after decryption.

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 net/tls/tls_sw.c | 88 ++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1003494d0563..367f5b0128f2 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1421,16 +1421,15 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 
 			return err;
 		}
+		rxm->offset += tls_ctx->rx.prepend_size;
+		rxm->full_len -= tls_ctx->rx.overhead_size;
+		tls_advance_record_sn(sk, &tls_ctx->rx);
+		ctx->decrypted = true;
+		ctx->saved_data_ready(sk);
 	} else {
 		*zc = false;
 	}
 
-	rxm->offset += tls_ctx->rx.prepend_size;
-	rxm->full_len -= tls_ctx->rx.overhead_size;
-	tls_advance_record_sn(sk, &tls_ctx->rx);
-	ctx->decrypted = true;
-	ctx->saved_data_ready(sk);
-
 	return err;
 }
 
@@ -1609,6 +1608,25 @@ int tls_sw_recvmsg(struct sock *sk,
 
 		rxm = strp_msg(skb);
 
+		to_decrypt = rxm->full_len - tls_ctx->rx.overhead_size;
+
+		if (to_decrypt <= len && !is_kvec && !is_peek &&
+		    ctx->control == TLS_RECORD_TYPE_DATA)
+			zc = true;
+
+		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
+					 &chunk, &zc, ctx->async_capable);
+		if (err < 0 && err != -EINPROGRESS) {
+			tls_err_abort(sk, EBADMSG);
+			goto recv_end;
+		}
+
+		if (err == -EINPROGRESS) {
+			async = true;
+			num_async++;
+			goto pick_next_record;
+		}
+
 		if (!cmsg) {
 			int cerr;
 
@@ -1626,40 +1644,22 @@ int tls_sw_recvmsg(struct sock *sk,
 			goto recv_end;
 		}
 
-		to_decrypt = rxm->full_len - tls_ctx->rx.overhead_size;
-
-		if (to_decrypt <= len && !is_kvec && !is_peek)
-			zc = true;
-
-		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
-					 &chunk, &zc, ctx->async_capable);
-		if (err < 0 && err != -EINPROGRESS) {
-			tls_err_abort(sk, EBADMSG);
-			goto recv_end;
-		}
-
-		if (err == -EINPROGRESS) {
-			async = true;
-			num_async++;
-			goto pick_next_record;
-		} else {
-			if (!zc) {
-				if (rxm->full_len > len) {
-					retain_skb = true;
-					chunk = len;
-				} else {
-					chunk = rxm->full_len;
-				}
+		if (!zc) {
+			if (rxm->full_len > len) {
+				retain_skb = true;
+				chunk = len;
+			} else {
+				chunk = rxm->full_len;
+			}
 
-				err = skb_copy_datagram_msg(skb, rxm->offset,
-							    msg, chunk);
-				if (err < 0)
-					goto recv_end;
+			err = skb_copy_datagram_msg(skb, rxm->offset,
+						    msg, chunk);
+			if (err < 0)
+				goto recv_end;
 
-				if (!is_peek) {
-					rxm->offset = rxm->offset + chunk;
-					rxm->full_len = rxm->full_len - chunk;
-				}
+			if (!is_peek) {
+				rxm->offset = rxm->offset + chunk;
+				rxm->full_len = rxm->full_len - chunk;
 			}
 		}
 
@@ -1759,15 +1759,15 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (!skb)
 		goto splice_read_end;
 
-	/* splice does not support reading control messages */
-	if (ctx->control != TLS_RECORD_TYPE_DATA) {
-		err = -ENOTSUPP;
-		goto splice_read_end;
-	}
-
 	if (!ctx->decrypted) {
 		err = decrypt_skb_update(sk, skb, NULL, &chunk, &zc, false);
 
+		/* splice does not support reading control messages */
+		if (ctx->control != TLS_RECORD_TYPE_DATA) {
+			err = -ENOTSUPP;
+			goto splice_read_end;
+		}
+
 		if (err < 0) {
 			tls_err_abort(sk, EBADMSG);
 			goto splice_read_end;
-- 
2.17.1


^ permalink raw reply related

* [PATCH net-next 6/7] net: tls: Add tls 1.3 support
From: Dave Watson @ 2019-01-24 17:21 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

TLS 1.3 has minor changes from TLS 1.2 at the record layer.

* Header now hardcodes the same version and application content type in
  the header.
* The real content type is appended after the data, before encryption (or
  after decryption).
* The IV is xored with the sequence number, instead of concatinating four
  bytes of IV with the explicit IV.
* Zero-padding:  No exlicit length is given, we search backwards from the
  end of the decrypted data for the first non-zero byte, which is the
  content type.  Currently recv supports reading zero-padding, but there
  is no way for send to add zero padding.

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 include/net/tls.h             |  66 ++++++++++++++-----
 include/uapi/linux/tls.h      |   4 ++
 net/tls/tls_device.c          |   5 +-
 net/tls/tls_device_fallback.c |   3 +-
 net/tls/tls_main.c            |   3 +-
 net/tls/tls_sw.c              | 116 +++++++++++++++++++++++++++-------
 6 files changed, 154 insertions(+), 43 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 9edb2cc7023a..a93a8ed8f716 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -119,6 +119,9 @@ struct tls_rec {
 	/* AAD | msg_encrypted.sg.data (data contains overhead for hdr & iv & tag) */
 	struct scatterlist sg_aead_out[2];
 
+	char content_type;
+	struct scatterlist sg_content_type;
+
 	char aad_space[TLS_AAD_SPACE_SIZE];
 	u8 iv_data[TLS_CIPHER_AES_GCM_128_IV_SIZE +
 		   TLS_CIPHER_AES_GCM_128_SALT_SIZE];
@@ -204,6 +207,7 @@ struct cipher_context {
 	u16 rec_seq_size;
 	char *rec_seq;
 	u16 aad_size;
+	u16 tail_size;
 };
 
 union tls_crypto_context {
@@ -398,49 +402,77 @@ static inline bool tls_bigint_increment(unsigned char *seq, int len)
 }
 
 static inline void tls_advance_record_sn(struct sock *sk,
-					 struct cipher_context *ctx)
+					 struct cipher_context *ctx,
+					 int version)
 {
 	if (tls_bigint_increment(ctx->rec_seq, ctx->rec_seq_size))
 		tls_err_abort(sk, EBADMSG);
-	tls_bigint_increment(ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
-			     ctx->iv_size);
+
+	if (version != TLS_1_3_VERSION) {
+		tls_bigint_increment(ctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
+				     ctx->iv_size);
+	}
 }
 
 static inline void tls_fill_prepend(struct tls_context *ctx,
 			     char *buf,
 			     size_t plaintext_len,
-			     unsigned char record_type)
+			     unsigned char record_type,
+			     int version)
 {
 	size_t pkt_len, iv_size = ctx->tx.iv_size;
 
-	pkt_len = plaintext_len + iv_size + ctx->tx.tag_size;
+	pkt_len = plaintext_len + ctx->tx.tag_size;
+	if (version != TLS_1_3_VERSION) {
+		pkt_len += iv_size;
+
+		memcpy(buf + TLS_NONCE_OFFSET,
+		       ctx->tx.iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv_size);
+	}
 
 	/* we cover nonce explicit here as well, so buf should be of
 	 * size KTLS_DTLS_HEADER_SIZE + KTLS_DTLS_NONCE_EXPLICIT_SIZE
 	 */
-	buf[0] = record_type;
-	buf[1] = TLS_VERSION_MINOR(ctx->crypto_send.info.version);
-	buf[2] = TLS_VERSION_MAJOR(ctx->crypto_send.info.version);
+	buf[0] = version == TLS_1_3_VERSION ?
+		   TLS_RECORD_TYPE_DATA : record_type;
+	/* Note that VERSION must be TLS_1_2 for both TLS1.2 and TLS1.3 */
+	buf[1] = TLS_1_2_VERSION_MINOR;
+	buf[2] = TLS_1_2_VERSION_MAJOR;
 	/* we can use IV for nonce explicit according to spec */
 	buf[3] = pkt_len >> 8;
 	buf[4] = pkt_len & 0xFF;
-	memcpy(buf + TLS_NONCE_OFFSET,
-	       ctx->tx.iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv_size);
 }
 
 static inline void tls_make_aad(char *buf,
 				size_t size,
 				char *record_sequence,
 				int record_sequence_size,
-				unsigned char record_type)
+				unsigned char record_type,
+				int version)
+{
+	if (version != TLS_1_3_VERSION) {
+		memcpy(buf, record_sequence, record_sequence_size);
+		buf += 8;
+	} else {
+		size += TLS_CIPHER_AES_GCM_128_TAG_SIZE;
+	}
+
+	buf[0] = version == TLS_1_3_VERSION ?
+		  TLS_RECORD_TYPE_DATA : record_type;
+	buf[1] = TLS_1_2_VERSION_MAJOR;
+	buf[2] = TLS_1_2_VERSION_MINOR;
+	buf[3] = size >> 8;
+	buf[4] = size & 0xFF;
+}
+
+static inline void xor_iv_with_seq(int version, char *iv, char *seq)
 {
-	memcpy(buf, record_sequence, record_sequence_size);
+	int i;
 
-	buf[8] = record_type;
-	buf[9] = TLS_1_2_VERSION_MAJOR;
-	buf[10] = TLS_1_2_VERSION_MINOR;
-	buf[11] = size >> 8;
-	buf[12] = size & 0xFF;
+	if (version == TLS_1_3_VERSION) {
+		for (i = 0; i < 8; i++)
+			iv[i + 4] ^= seq[i];
+	}
 }
 
 static inline struct tls_context *tls_get_ctx(const struct sock *sk)
diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index 9affceaa3db4..401d6f01de6a 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -51,6 +51,10 @@
 #define TLS_1_2_VERSION_MINOR	0x3
 #define TLS_1_2_VERSION		TLS_VERSION_NUMBER(TLS_1_2)
 
+#define TLS_1_3_VERSION_MAJOR	0x3
+#define TLS_1_3_VERSION_MINOR	0x4
+#define TLS_1_3_VERSION		TLS_VERSION_NUMBER(TLS_1_3)
+
 /* Supported ciphers */
 #define TLS_CIPHER_AES_GCM_128				51
 #define TLS_CIPHER_AES_GCM_128_IV_SIZE			8
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index d753e362d2d9..7ee9008b2187 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -257,7 +257,8 @@ static int tls_push_record(struct sock *sk,
 	tls_fill_prepend(ctx,
 			 skb_frag_address(frag),
 			 record->len - ctx->tx.prepend_size,
-			 record_type);
+			 record_type,
+			 ctx->crypto_send.info.version);
 
 	/* HW doesn't care about the data in the tag, because it fills it. */
 	dummy_tag_frag.page = skb_frag_page(frag);
@@ -270,7 +271,7 @@ static int tls_push_record(struct sock *sk,
 	spin_unlock_irq(&offload_ctx->lock);
 	offload_ctx->open_record = NULL;
 	set_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
-	tls_advance_record_sn(sk, &ctx->tx);
+	tls_advance_record_sn(sk, &ctx->tx, ctx->crypto_send.info.version);
 
 	for (i = 0; i < record->num_frags; i++) {
 		frag = &record->frags[i];
diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 450a6dbc5a88..54c3a758f2a7 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -73,7 +73,8 @@ static int tls_enc_record(struct aead_request *aead_req,
 	len -= TLS_CIPHER_AES_GCM_128_IV_SIZE;
 
 	tls_make_aad(aad, len - TLS_CIPHER_AES_GCM_128_TAG_SIZE,
-		     (char *)&rcd_sn, sizeof(rcd_sn), buf[0]);
+		(char *)&rcd_sn, sizeof(rcd_sn), buf[0],
+		TLS_1_2_VERSION);
 
 	memcpy(iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, buf + TLS_HEADER_SIZE,
 	       TLS_CIPHER_AES_GCM_128_IV_SIZE);
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 0f028cfdf835..d1c2fd9a3f63 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -463,7 +463,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, char __user *optval,
 	}
 
 	/* check version */
-	if (crypto_info->version != TLS_1_2_VERSION) {
+	if (crypto_info->version != TLS_1_2_VERSION &&
+	    crypto_info->version != TLS_1_3_VERSION) {
 		rc = -ENOTSUPP;
 		goto err_crypto_info;
 	}
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 367f5b0128f2..5e46112c9efd 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -120,6 +120,34 @@ static int skb_nsg(struct sk_buff *skb, int offset, int len)
         return __skb_nsg(skb, offset, len, 0);
 }
 
+static int padding_length(struct tls_sw_context_rx *ctx,
+			  struct tls_context *tls_ctx, struct sk_buff *skb)
+{
+	struct strp_msg *rxm = strp_msg(skb);
+	int sub = 0;
+
+	/* Determine zero-padding length */
+	if (tls_ctx->crypto_recv.info.version == TLS_1_3_VERSION) {
+		char content_type = 0;
+		int err;
+		int back = 17;
+
+		while (content_type == 0) {
+			if (back > rxm->full_len)
+				return -EBADMSG;
+			err = skb_copy_bits(skb,
+					    rxm->offset + rxm->full_len - back,
+					    &content_type, 1);
+			if (content_type)
+				break;
+			sub++;
+			back++;
+		}
+		ctx->control = content_type;
+	}
+	return sub;
+}
+
 static void tls_decrypt_done(struct crypto_async_request *req, int err)
 {
 	struct aead_request *aead_req = (struct aead_request *)req;
@@ -142,7 +170,7 @@ static void tls_decrypt_done(struct crypto_async_request *req, int err)
 		tls_err_abort(skb->sk, err);
 	} else {
 		struct strp_msg *rxm = strp_msg(skb);
-
+		rxm->full_len -= padding_length(ctx, tls_ctx, skb);
 		rxm->offset += tls_ctx->rx.prepend_size;
 		rxm->full_len -= tls_ctx->rx.overhead_size;
 	}
@@ -448,6 +476,8 @@ static int tls_do_encryption(struct sock *sk,
 	int rc;
 
 	memcpy(rec->iv_data, tls_ctx->tx.iv, sizeof(rec->iv_data));
+	xor_iv_with_seq(tls_ctx->crypto_send.info.version, rec->iv_data,
+			tls_ctx->tx.rec_seq);
 
 	sge->offset += tls_ctx->tx.prepend_size;
 	sge->length -= tls_ctx->tx.prepend_size;
@@ -483,7 +513,8 @@ static int tls_do_encryption(struct sock *sk,
 
 	/* Unhook the record from context if encryption is not failure */
 	ctx->open_rec = NULL;
-	tls_advance_record_sn(sk, &tls_ctx->tx);
+	tls_advance_record_sn(sk, &tls_ctx->tx,
+			      tls_ctx->crypto_send.info.version);
 	return rc;
 }
 
@@ -640,7 +671,17 @@ static int tls_push_record(struct sock *sk, int flags,
 
 	i = msg_pl->sg.end;
 	sk_msg_iter_var_prev(i);
-	sg_mark_end(sk_msg_elem(msg_pl, i));
+
+	rec->content_type = record_type;
+	if (tls_ctx->crypto_send.info.version == TLS_1_3_VERSION) {
+		/* Add content type to end of message.  No padding added */
+		sg_set_buf(&rec->sg_content_type, &rec->content_type, 1);
+		sg_mark_end(&rec->sg_content_type);
+		sg_chain(msg_pl->sg.data, msg_pl->sg.end + 1,
+			 &rec->sg_content_type);
+	} else {
+		sg_mark_end(sk_msg_elem(msg_pl, i));
+	}
 
 	i = msg_pl->sg.start;
 	sg_chain(rec->sg_aead_in, 2, rec->inplace_crypto ?
@@ -653,18 +694,22 @@ static int tls_push_record(struct sock *sk, int flags,
 	i = msg_en->sg.start;
 	sg_chain(rec->sg_aead_out, 2, &msg_en->sg.data[i]);
 
-	tls_make_aad(rec->aad_space, msg_pl->sg.size,
+	tls_make_aad(rec->aad_space, msg_pl->sg.size + tls_ctx->tx.tail_size,
 		     tls_ctx->tx.rec_seq, tls_ctx->tx.rec_seq_size,
-		     record_type);
+		     record_type,
+		     tls_ctx->crypto_send.info.version);
 
 	tls_fill_prepend(tls_ctx,
 			 page_address(sg_page(&msg_en->sg.data[i])) +
-			 msg_en->sg.data[i].offset, msg_pl->sg.size,
-			 record_type);
+			 msg_en->sg.data[i].offset,
+			 msg_pl->sg.size + tls_ctx->tx.tail_size,
+			 record_type,
+			 tls_ctx->crypto_send.info.version);
 
 	tls_ctx->pending_open_record_frags = false;
 
-	rc = tls_do_encryption(sk, tls_ctx, ctx, req, msg_pl->sg.size, i);
+	rc = tls_do_encryption(sk, tls_ctx, ctx, req,
+			       msg_pl->sg.size + tls_ctx->tx.tail_size, i);
 	if (rc < 0) {
 		if (rc != -EINPROGRESS) {
 			tls_err_abort(sk, EBADMSG);
@@ -1292,7 +1337,8 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	u8 *aad, *iv, *mem = NULL;
 	struct scatterlist *sgin = NULL;
 	struct scatterlist *sgout = NULL;
-	const int data_len = rxm->full_len - tls_ctx->rx.overhead_size;
+	const int data_len = rxm->full_len - tls_ctx->rx.overhead_size +
+		tls_ctx->rx.tail_size;
 
 	if (*zc && (out_iov || out_sg)) {
 		if (out_iov)
@@ -1343,12 +1389,20 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 		kfree(mem);
 		return err;
 	}
-	memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
+	if (tls_ctx->crypto_recv.info.version == TLS_1_3_VERSION)
+		memcpy(iv, tls_ctx->rx.iv, crypto_aead_ivsize(ctx->aead_recv));
+	else
+		memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
+
+	xor_iv_with_seq(tls_ctx->crypto_recv.info.version, iv,
+			tls_ctx->rx.rec_seq);
 
 	/* Prepare AAD */
-	tls_make_aad(aad, rxm->full_len - tls_ctx->rx.overhead_size,
+	tls_make_aad(aad, rxm->full_len - tls_ctx->rx.overhead_size +
+		     tls_ctx->rx.tail_size,
 		     tls_ctx->rx.rec_seq, tls_ctx->rx.rec_seq_size,
-		     ctx->control);
+		     ctx->control,
+		     tls_ctx->crypto_recv.info.version);
 
 	/* Prepare sgin */
 	sg_init_table(sgin, n_sgin);
@@ -1405,6 +1459,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 {
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
+	int version = tls_ctx->crypto_recv.info.version;
 	struct strp_msg *rxm = strp_msg(skb);
 	int err = 0;
 
@@ -1417,13 +1472,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
 		err = decrypt_internal(sk, skb, dest, NULL, chunk, zc, async);
 		if (err < 0) {
 			if (err == -EINPROGRESS)
-				tls_advance_record_sn(sk, &tls_ctx->rx);
+				tls_advance_record_sn(sk, &tls_ctx->rx,
+						      version);
 
 			return err;
 		}
+
+		rxm->full_len -= padding_length(ctx, tls_ctx, skb);
+
 		rxm->offset += tls_ctx->rx.prepend_size;
 		rxm->full_len -= tls_ctx->rx.overhead_size;
-		tls_advance_record_sn(sk, &tls_ctx->rx);
+		tls_advance_record_sn(sk, &tls_ctx->rx, version);
 		ctx->decrypted = true;
 		ctx->saved_data_ready(sk);
 	} else {
@@ -1611,7 +1670,8 @@ int tls_sw_recvmsg(struct sock *sk,
 		to_decrypt = rxm->full_len - tls_ctx->rx.overhead_size;
 
 		if (to_decrypt <= len && !is_kvec && !is_peek &&
-		    ctx->control == TLS_RECORD_TYPE_DATA)
+		    ctx->control == TLS_RECORD_TYPE_DATA &&
+		    tls_ctx->crypto_recv.info.version != TLS_1_3_VERSION)
 			zc = true;
 
 		err = decrypt_skb_update(sk, skb, &msg->msg_iter,
@@ -1835,9 +1895,12 @@ static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
 
 	data_len = ((header[4] & 0xFF) | (header[3] << 8));
 
-	cipher_overhead = tls_ctx->rx.tag_size + tls_ctx->rx.iv_size;
+	cipher_overhead = tls_ctx->rx.tag_size;
+	if (tls_ctx->crypto_recv.info.version != TLS_1_3_VERSION)
+		cipher_overhead += tls_ctx->rx.iv_size;
 
-	if (data_len > TLS_MAX_PAYLOAD_SIZE + cipher_overhead) {
+	if (data_len > TLS_MAX_PAYLOAD_SIZE + cipher_overhead +
+	    tls_ctx->rx.tail_size) {
 		ret = -EMSGSIZE;
 		goto read_failure;
 	}
@@ -1846,12 +1909,12 @@ static int tls_read_size(struct strparser *strp, struct sk_buff *skb)
 		goto read_failure;
 	}
 
-	if (header[1] != TLS_VERSION_MINOR(tls_ctx->crypto_recv.info.version) ||
-	    header[2] != TLS_VERSION_MAJOR(tls_ctx->crypto_recv.info.version)) {
+	/* Note that both TLS1.3 and TLS1.2 use TLS_1_2 version here */
+	if (header[1] != TLS_1_2_VERSION_MINOR ||
+	    header[2] != TLS_1_2_VERSION_MAJOR) {
 		ret = -EINVAL;
 		goto read_failure;
 	}
-
 #ifdef CONFIG_TLS_DEVICE
 	handle_device_resync(strp->sk, TCP_SKB_CB(skb)->seq + rxm->offset,
 			     *(u64*)tls_ctx->rx.rec_seq);
@@ -2098,10 +2161,19 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 		goto free_priv;
 	}
 
-	cctx->aad_size = TLS_AAD_SPACE_SIZE;
+	if (crypto_info->version == TLS_1_3_VERSION) {
+		nonce_size = 0;
+		cctx->aad_size = TLS_HEADER_SIZE;
+		cctx->tail_size = 1;
+	} else {
+		cctx->aad_size = TLS_AAD_SPACE_SIZE;
+		cctx->tail_size = 0;
+	}
+
 	cctx->prepend_size = TLS_HEADER_SIZE + nonce_size;
 	cctx->tag_size = tag_size;
-	cctx->overhead_size = cctx->prepend_size + cctx->tag_size;
+	cctx->overhead_size = cctx->prepend_size + cctx->tag_size +
+		cctx->tail_size;
 	cctx->iv_size = iv_size;
 	cctx->iv = kmalloc(iv_size + TLS_CIPHER_AES_GCM_128_SALT_SIZE,
 			   GFP_KERNEL);
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH net-next v3 2/2] net/mlx5e: Don't overwrite pedit action when multiple pedit used
From: Or Gerlitz @ 2019-01-24 17:21 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Saeed Mahameed, Linux Netdev List
In-Reply-To: <1548156126-8021-2-git-send-email-xiangxia.m.yue@gmail.com>

On Tue, Jan 22, 2019 at 4:38 PM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> In some case, we may use multiple pedit actions to modify packets.
> The command shown as below: the last pedit action is effective.
>
> $ tc filter add dev netdev_rep parent ffff: protocol ip prio 1  \
>         flower skip_sw ip_proto icmp dst_ip 3.3.3.3                     \
>         action pedit ex munge ip dst set 192.168.1.100 pipe             \
>         action pedit ex munge eth src set 00:00:00:00:00:01 pipe        \
>         action pedit ex munge eth dst set 00:00:00:00:00:02 pipe        \
>         action csum ip pipe                                             \
>         action tunnel_key set src_ip 1.1.1.100 dst_ip 1.1.1.200 dst_port 4789 id 100 \
>         action mirred egress redirect dev vxlan0
>
> To fix it, we add max_mod_hdr_actions to mlx5e_tc_flow_parse_attr struction,
> max_mod_hdr_actions will store the max pedit action number we support and
> num_mod_hdr_actions indicates how many pedit action we used, and store all
> pedit action to mod_hdr_actions.

There's a comment just above offload_pedit_fields saying:

/* On input attr->num_mod_hdr_actions tells how many HW actions can be parsed at
 * max from the SW pedit action. On success, it says how many HW actions were
 * actually parsed.
 */

I guess it will have to change now, right?

^ permalink raw reply

* [PATCH net-next 4/7] net: tls: Refactor tls aad space size calculation
From: Dave Watson @ 2019-01-24 17:20 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

TLS 1.3 has a different AAD size, use a variable in the code to
make TLS 1.3 support easy.

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 include/net/tls.h |  1 +
 net/tls/tls_sw.c  | 17 +++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 675161ed37eb..9edb2cc7023a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -203,6 +203,7 @@ struct cipher_context {
 	char *iv;
 	u16 rec_seq_size;
 	char *rec_seq;
+	u16 aad_size;
 };
 
 union tls_crypto_context {
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 706ff02dc379..1003494d0563 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -185,7 +185,7 @@ static int tls_do_decryption(struct sock *sk,
 	int ret;
 
 	aead_request_set_tfm(aead_req, ctx->aead_recv);
-	aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
+	aead_request_set_ad(aead_req, tls_ctx->rx.aad_size);
 	aead_request_set_crypt(aead_req, sgin, sgout,
 			       data_len + tls_ctx->rx.tag_size,
 			       (u8 *)iv_recv);
@@ -289,12 +289,12 @@ static struct tls_rec *tls_get_rec(struct sock *sk)
 
 	sg_init_table(rec->sg_aead_in, 2);
 	sg_set_buf(&rec->sg_aead_in[0], rec->aad_space,
-		   sizeof(rec->aad_space));
+		   tls_ctx->tx.aad_size);
 	sg_unmark_end(&rec->sg_aead_in[1]);
 
 	sg_init_table(rec->sg_aead_out, 2);
 	sg_set_buf(&rec->sg_aead_out[0], rec->aad_space,
-		   sizeof(rec->aad_space));
+		   tls_ctx->tx.aad_size);
 	sg_unmark_end(&rec->sg_aead_out[1]);
 
 	return rec;
@@ -455,7 +455,7 @@ static int tls_do_encryption(struct sock *sk,
 	msg_en->sg.curr = start;
 
 	aead_request_set_tfm(aead_req, ctx->aead_send);
-	aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
+	aead_request_set_ad(aead_req, tls_ctx->tx.aad_size);
 	aead_request_set_crypt(aead_req, rec->sg_aead_in,
 			       rec->sg_aead_out,
 			       data_len, rec->iv_data);
@@ -1317,7 +1317,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 
 	aead_size = sizeof(*aead_req) + crypto_aead_reqsize(ctx->aead_recv);
 	mem_size = aead_size + (nsg * sizeof(struct scatterlist));
-	mem_size = mem_size + TLS_AAD_SPACE_SIZE;
+	mem_size = mem_size + tls_ctx->rx.aad_size;
 	mem_size = mem_size + crypto_aead_ivsize(ctx->aead_recv);
 
 	/* Allocate a single block of memory which contains
@@ -1333,7 +1333,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	sgin = (struct scatterlist *)(mem + aead_size);
 	sgout = sgin + n_sgin;
 	aad = (u8 *)(sgout + n_sgout);
-	iv = aad + TLS_AAD_SPACE_SIZE;
+	iv = aad + tls_ctx->rx.aad_size;
 
 	/* Prepare IV */
 	err = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,
@@ -1352,7 +1352,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 
 	/* Prepare sgin */
 	sg_init_table(sgin, n_sgin);
-	sg_set_buf(&sgin[0], aad, TLS_AAD_SPACE_SIZE);
+	sg_set_buf(&sgin[0], aad, tls_ctx->rx.aad_size);
 	err = skb_to_sgvec(skb, &sgin[1],
 			   rxm->offset + tls_ctx->rx.prepend_size,
 			   rxm->full_len - tls_ctx->rx.prepend_size);
@@ -1364,7 +1364,7 @@ static int decrypt_internal(struct sock *sk, struct sk_buff *skb,
 	if (n_sgout) {
 		if (out_iov) {
 			sg_init_table(sgout, n_sgout);
-			sg_set_buf(&sgout[0], aad, TLS_AAD_SPACE_SIZE);
+			sg_set_buf(&sgout[0], aad, tls_ctx->rx.aad_size);
 
 			*chunk = 0;
 			err = tls_setup_from_iter(sk, out_iov, data_len,
@@ -2098,6 +2098,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 		goto free_priv;
 	}
 
+	cctx->aad_size = TLS_AAD_SPACE_SIZE;
 	cctx->prepend_size = TLS_HEADER_SIZE + nonce_size;
 	cctx->tag_size = tag_size;
 	cctx->overhead_size = cctx->prepend_size + cctx->tag_size;
-- 
2.17.1


^ permalink raw reply related

* Re: tc filter insertion rate degradation
From: Dennis Zhou @ 2019-01-24 17:21 UTC (permalink / raw)
  To: Eric Dumazet, Vlad Buslov
  Cc: Tejun Heo, Linux Kernel Network Developers, Yevgeny Kliteynik,
	Yossef Efraim, Maor Gottlieb
In-Reply-To: <CANn89iKb_vW+LA-91RV=zuAqbNycPFUYW54w_S=KZ3HdcWPw6Q@mail.gmail.com>

Hi Vlad and Eric,

On Tue, Jan 22, 2019 at 09:33:10AM -0800, Eric Dumazet wrote:
> On Mon, Jan 21, 2019 at 3:24 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >
> > Hi Eric,
> >
> > I've been investigating significant tc filter insertion rate degradation
> > and it seems it is caused by your commit 001c96db0181 ("net: align
> > gnet_stats_basic_cpu struct"). With this commit insertion rate is
> > reduced from ~65k rules/sec to ~43k rules/sec when inserting 1m rules
> > from file in tc batch mode on my machine.
> >
> > Tc perf profile indicates that pcpu allocator now consumes 2x CPU:
> >
> > 1) Before:
> >
> > Samples: 63K of event 'cycles:ppp', Event count (approx.): 48796480071
> >   Children      Self  Co  Shared Object     Symbol
> > +   21.19%     3.38%  tc  [kernel.vmlinux]  [k] pcpu_alloc
> > +    3.45%     0.25%  tc  [kernel.vmlinux]  [k] pcpu_alloc_area
> >
> > 2) After:
> >
> > Samples1: 92K of event 'cycles:ppp', Event count (approx.): 71446806550
> >   Children      Self  Co  Shared Object     Symbol
> > +   44.67%     3.99%  tc  [kernel.vmlinux]  [k] pcpu_alloc
> > +   19.25%     0.22%  tc  [kernel.vmlinux]  [k] pcpu_alloc_area
> >
> > It seems that it takes much more work for pcpu allocator to perform
> > allocation with new stricter alignment requirements. Not sure if it is
> > expected behavior or not in this case.
> >
> > Regards,
> > Vlad

Would you mind sharing a little more information with me:
1) output before and after a run of /sys/kernel/debug/percpu_stats
2) a full perf output
3) a reproducer

I'm a little surprised we're spending time in pcpu_alloc_area(), but it
might be due to constantly breaking the hint as an immediate guess.

> 
> Hi Vlad
> 
> I guess this is more a question for per-cpu allocator experts / maintainers ?
> 
> 16-bytes alignment for 16-bytes objects sound quite reasonable [1]
> 

The alignment request seems reasonable. But as Tejun mentioned in a
reply to this, the overhead of forced alignment would be both in percpu
memory itself and in allocation time due to the stricter requirement.

Thanks,
Dennis

^ permalink raw reply

* [PATCH net-next 3/7] net: tls: Support 256 bit keys
From: Dave Watson @ 2019-01-24 17:20 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

Wire up support for 256 bit keys from the setsockopt to the crypto framework

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 include/net/tls.h                 |  5 ++-
 include/uapi/linux/tls.h          | 15 ++++++++
 net/tls/tls_main.c                | 33 +++++++++++++++-
 net/tls/tls_sw.c                  | 29 +++++++++++++--
 tools/testing/selftests/net/tls.c | 62 +++++++++++++++++++++++++++++++
 5 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index eb73e62ac8c9..675161ed37eb 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -207,7 +207,10 @@ struct cipher_context {
 
 union tls_crypto_context {
 	struct tls_crypto_info info;
-	struct tls12_crypto_info_aes_gcm_128 aes_gcm_128;
+	union {
+		struct tls12_crypto_info_aes_gcm_128 aes_gcm_128;
+		struct tls12_crypto_info_aes_gcm_256 aes_gcm_256;
+	};
 };
 
 struct tls_context {
diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index ff02287495ac..9affceaa3db4 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -59,6 +59,13 @@
 #define TLS_CIPHER_AES_GCM_128_TAG_SIZE		16
 #define TLS_CIPHER_AES_GCM_128_REC_SEQ_SIZE		8
 
+#define TLS_CIPHER_AES_GCM_256				52
+#define TLS_CIPHER_AES_GCM_256_IV_SIZE			8
+#define TLS_CIPHER_AES_GCM_256_KEY_SIZE		32
+#define TLS_CIPHER_AES_GCM_256_SALT_SIZE		4
+#define TLS_CIPHER_AES_GCM_256_TAG_SIZE		16
+#define TLS_CIPHER_AES_GCM_256_REC_SEQ_SIZE		8
+
 #define TLS_SET_RECORD_TYPE	1
 #define TLS_GET_RECORD_TYPE	2
 
@@ -75,4 +82,12 @@ struct tls12_crypto_info_aes_gcm_128 {
 	unsigned char rec_seq[TLS_CIPHER_AES_GCM_128_REC_SEQ_SIZE];
 };
 
+struct tls12_crypto_info_aes_gcm_256 {
+	struct tls_crypto_info info;
+	unsigned char iv[TLS_CIPHER_AES_GCM_256_IV_SIZE];
+	unsigned char key[TLS_CIPHER_AES_GCM_256_KEY_SIZE];
+	unsigned char salt[TLS_CIPHER_AES_GCM_256_SALT_SIZE];
+	unsigned char rec_seq[TLS_CIPHER_AES_GCM_256_REC_SEQ_SIZE];
+};
+
 #endif /* _UAPI_LINUX_TLS_H */
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index d36d095cbcf0..0f028cfdf835 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -372,6 +372,30 @@ static int do_tls_getsockopt_tx(struct sock *sk, char __user *optval,
 			rc = -EFAULT;
 		break;
 	}
+	case TLS_CIPHER_AES_GCM_256: {
+		struct tls12_crypto_info_aes_gcm_256 *
+		  crypto_info_aes_gcm_256 =
+		  container_of(crypto_info,
+			       struct tls12_crypto_info_aes_gcm_256,
+			       info);
+
+		if (len != sizeof(*crypto_info_aes_gcm_256)) {
+			rc = -EINVAL;
+			goto out;
+		}
+		lock_sock(sk);
+		memcpy(crypto_info_aes_gcm_256->iv,
+		       ctx->tx.iv + TLS_CIPHER_AES_GCM_256_SALT_SIZE,
+		       TLS_CIPHER_AES_GCM_256_IV_SIZE);
+		memcpy(crypto_info_aes_gcm_256->rec_seq, ctx->tx.rec_seq,
+		       TLS_CIPHER_AES_GCM_256_REC_SEQ_SIZE);
+		release_sock(sk);
+		if (copy_to_user(optval,
+				 crypto_info_aes_gcm_256,
+				 sizeof(*crypto_info_aes_gcm_256)))
+			rc = -EFAULT;
+		break;
+	}
 	default:
 		rc = -EINVAL;
 	}
@@ -412,6 +436,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, char __user *optval,
 {
 	struct tls_crypto_info *crypto_info;
 	struct tls_context *ctx = tls_get_ctx(sk);
+	size_t optsize;
 	int rc = 0;
 	int conf;
 
@@ -444,8 +469,12 @@ static int do_tls_setsockopt_conf(struct sock *sk, char __user *optval,
 	}
 
 	switch (crypto_info->cipher_type) {
-	case TLS_CIPHER_AES_GCM_128: {
-		if (optlen != sizeof(struct tls12_crypto_info_aes_gcm_128)) {
+	case TLS_CIPHER_AES_GCM_128:
+	case TLS_CIPHER_AES_GCM_256: {
+		optsize = crypto_info->cipher_type == TLS_CIPHER_AES_GCM_128 ?
+			sizeof(struct tls12_crypto_info_aes_gcm_128) :
+			sizeof(struct tls12_crypto_info_aes_gcm_256);
+		if (optlen != optsize) {
 			rc = -EINVAL;
 			goto err_crypto_info;
 		}
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index acfafe8190a3..706ff02dc379 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1997,6 +1997,7 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 {
 	struct tls_crypto_info *crypto_info;
 	struct tls12_crypto_info_aes_gcm_128 *gcm_128_info;
+	struct tls12_crypto_info_aes_gcm_256 *gcm_256_info;
 	struct tls_sw_context_tx *sw_ctx_tx = NULL;
 	struct tls_sw_context_rx *sw_ctx_rx = NULL;
 	struct cipher_context *cctx;
@@ -2004,7 +2005,8 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 	struct strp_callbacks cb;
 	u16 nonce_size, tag_size, iv_size, rec_seq_size;
 	struct crypto_tfm *tfm;
-	char *iv, *rec_seq;
+	char *iv, *rec_seq, *key, *salt;
+	size_t keysize;
 	int rc = 0;
 
 	if (!ctx) {
@@ -2065,6 +2067,24 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 		 ((struct tls12_crypto_info_aes_gcm_128 *)crypto_info)->rec_seq;
 		gcm_128_info =
 			(struct tls12_crypto_info_aes_gcm_128 *)crypto_info;
+		keysize = TLS_CIPHER_AES_GCM_128_KEY_SIZE;
+		key = gcm_128_info->key;
+		salt = gcm_128_info->salt;
+		break;
+	}
+	case TLS_CIPHER_AES_GCM_256: {
+		nonce_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
+		tag_size = TLS_CIPHER_AES_GCM_256_TAG_SIZE;
+		iv_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
+		iv = ((struct tls12_crypto_info_aes_gcm_256 *)crypto_info)->iv;
+		rec_seq_size = TLS_CIPHER_AES_GCM_256_REC_SEQ_SIZE;
+		rec_seq =
+		 ((struct tls12_crypto_info_aes_gcm_256 *)crypto_info)->rec_seq;
+		gcm_256_info =
+			(struct tls12_crypto_info_aes_gcm_256 *)crypto_info;
+		keysize = TLS_CIPHER_AES_GCM_256_KEY_SIZE;
+		key = gcm_256_info->key;
+		salt = gcm_256_info->salt;
 		break;
 	}
 	default:
@@ -2088,7 +2108,8 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 		rc = -ENOMEM;
 		goto free_priv;
 	}
-	memcpy(cctx->iv, gcm_128_info->salt, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
+	/* Note: 128 & 256 bit salt are the same size */
+	memcpy(cctx->iv, salt, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
 	memcpy(cctx->iv + TLS_CIPHER_AES_GCM_128_SALT_SIZE, iv, iv_size);
 	cctx->rec_seq_size = rec_seq_size;
 	cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
@@ -2108,8 +2129,8 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 
 	ctx->push_pending_record = tls_sw_push_pending_record;
 
-	rc = crypto_aead_setkey(*aead, gcm_128_info->key,
-				TLS_CIPHER_AES_GCM_128_KEY_SIZE);
+	rc = crypto_aead_setkey(*aead, key, keysize);
+
 	if (rc)
 		goto free_aead;
 
diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index ff68ed19c0ef..c356f481de79 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -763,4 +763,66 @@ TEST_F(tls, control_msg)
 	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
 }
 
+TEST(keysizes) {
+	struct tls12_crypto_info_aes_gcm_256 tls12;
+	struct sockaddr_in addr;
+	int sfd, ret, fd, cfd;
+	socklen_t len;
+	bool notls;
+
+	notls = false;
+	len = sizeof(addr);
+
+	memset(&tls12, 0, sizeof(tls12));
+	tls12.info.version = TLS_1_2_VERSION;
+	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_256;
+
+	addr.sin_family = AF_INET;
+	addr.sin_addr.s_addr = htonl(INADDR_ANY);
+	addr.sin_port = 0;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	sfd = socket(AF_INET, SOCK_STREAM, 0);
+
+	ret = bind(sfd, &addr, sizeof(addr));
+	ASSERT_EQ(ret, 0);
+	ret = listen(sfd, 10);
+	ASSERT_EQ(ret, 0);
+
+	ret = getsockname(sfd, &addr, &len);
+	ASSERT_EQ(ret, 0);
+
+	ret = connect(fd, &addr, sizeof(addr));
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
+	if (ret != 0) {
+		notls = true;
+		printf("Failure setting TCP_ULP, testing without tls\n");
+	}
+
+	if (!notls) {
+		ret = setsockopt(fd, SOL_TLS, TLS_TX, &tls12,
+				 sizeof(tls12));
+		EXPECT_EQ(ret, 0);
+	}
+
+	cfd = accept(sfd, &addr, &len);
+	ASSERT_GE(cfd, 0);
+
+	if (!notls) {
+		ret = setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls",
+				 sizeof("tls"));
+		EXPECT_EQ(ret, 0);
+
+		ret = setsockopt(cfd, SOL_TLS, TLS_RX, &tls12,
+				 sizeof(tls12));
+		EXPECT_EQ(ret, 0);
+	}
+
+	close(sfd);
+	close(fd);
+	close(cfd);
+}
+
 TEST_HARNESS_MAIN
-- 
2.17.1


^ permalink raw reply related

* [PATCH net-next 1/7] net: tls: Save iv in tls_rec for async crypto requests
From: Dave Watson @ 2019-01-24 17:20 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

aead_request_set_crypt takes an iv pointer, and we change the iv
soon after setting it.  Some async crypto algorithms don't save the iv,
so we need to save it in the tls_rec for async requests.

Found by hardcoding x64 aesni to use async crypto manager (to test the async
codepath), however I don't think this combination can happen in the wild.
Presumably other hardware offloads will need this fix, but there have been
no user reports.

Fixes: a42055e8d2c30 ("Add support for async encryption of records...")
Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 include/net/tls.h | 2 ++
 net/tls/tls_sw.c  | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 90bf52db573e..4592606e136a 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -120,6 +120,8 @@ struct tls_rec {
 	struct scatterlist sg_aead_out[2];
 
 	char aad_space[TLS_AAD_SPACE_SIZE];
+	u8 iv_data[TLS_CIPHER_AES_GCM_128_IV_SIZE +
+		   TLS_CIPHER_AES_GCM_128_SALT_SIZE];
 	struct aead_request aead_req;
 	u8 aead_req_ctx[];
 };
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 86b9527c4826..0382a4674bb3 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -447,6 +447,8 @@ static int tls_do_encryption(struct sock *sk,
 	struct scatterlist *sge = sk_msg_elem(msg_en, start);
 	int rc;
 
+	memcpy(rec->iv_data, tls_ctx->tx.iv, sizeof(rec->iv_data));
+
 	sge->offset += tls_ctx->tx.prepend_size;
 	sge->length -= tls_ctx->tx.prepend_size;
 
@@ -456,7 +458,7 @@ static int tls_do_encryption(struct sock *sk,
 	aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
 	aead_request_set_crypt(aead_req, rec->sg_aead_in,
 			       rec->sg_aead_out,
-			       data_len, tls_ctx->tx.iv);
+			       data_len, rec->iv_data);
 
 	aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				  tls_encrypt_done, sk);
-- 
2.17.1


^ permalink raw reply related

* [PATCH net-next 2/7] net: tls: Set async_capable for tls zerocopy only if we see EINPROGRESS
From: Dave Watson @ 2019-01-24 17:20 UTC (permalink / raw)
  To: netdev@vger.kernel.org, Dave Miller
  Cc: Vakul Garg, Boris Pismenny, Aviad Yehezkel, John Fastabend,
	Daniel Borkmann

Currently we don't zerocopy if the crypto framework async bit is set.
However some crypto algorithms (such as x86 AESNI) support async,
but in the context of sendmsg, will never run asynchronously.  Instead,
check for actual EINPROGRESS return code before assuming algorithm is
async.

Signed-off-by: Dave Watson <davejwatson@fb.com>
---
 include/net/tls.h | 1 +
 net/tls/tls_sw.c  | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index 4592606e136a..eb73e62ac8c9 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -139,6 +139,7 @@ struct tls_sw_context_tx {
 	struct list_head tx_list;
 	atomic_t encrypt_pending;
 	int async_notify;
+	int async_capable;
 
 #define BIT_TX_SCHEDULED	0
 	unsigned long tx_bitmask;
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 0382a4674bb3..acfafe8190a3 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -673,6 +673,7 @@ static int tls_push_record(struct sock *sk, int flags,
 				tls_merge_open_record(sk, rec, tmp, orig_end);
 			}
 		}
+		ctx->async_capable = 1;
 		return rc;
 	} else if (split) {
 		msg_pl = &tmp->msg_plaintext;
@@ -814,8 +815,7 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 	long timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 	struct tls_context *tls_ctx = tls_get_ctx(sk);
 	struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
-	struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
-	bool async_capable = tfm->__crt_alg->cra_flags & CRYPTO_ALG_ASYNC;
+	bool async_capable = ctx->async_capable;
 	unsigned char record_type = TLS_RECORD_TYPE_DATA;
 	bool is_kvec = iov_iter_is_kvec(&msg->msg_iter);
 	bool eor = !(msg->msg_flags & MSG_MORE);
-- 
2.17.1


^ permalink raw reply related

* [PATCH net] ibmveth: Do not process frames after calling napi_reschedule
From: Thomas Falcon @ 2019-01-24 17:17 UTC (permalink / raw)
  To: netdev; +Cc: Thomas Falcon

The IBM virtual ethernet driver's polling function continues
to process frames after rescheduling NAPI, resulting in a warning
if it exhausted its budget. Do not restart polling after calling
napi_reschedule. Instead let frames be processed in the following
instance.

Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 098d876..dd71d5d 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1313,7 +1313,6 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 	unsigned long lpar_rc;
 	u16 mss = 0;
 
-restart_poll:
 	while (frames_processed < budget) {
 		if (!ibmveth_rxq_pending_buffer(adapter))
 			break;
@@ -1401,7 +1400,6 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 		    napi_reschedule(napi)) {
 			lpar_rc = h_vio_signal(adapter->vdev->unit_address,
 					       VIO_IRQ_DISABLE);
-			goto restart_poll;
 		}
 	}
 
-- 
1.8.3.1


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox