* Re: [PATCH net v3 0/2] SCTP PMTU discovery fixes
From: Xin Long @ 2018-01-06 5:05 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: network dev, linux-sctp, Vlad Yasevich, Neil Horman
In-Reply-To: <cover.1515152627.git.marcelo.leitner@gmail.com>
On Fri, Jan 5, 2018 at 9:17 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> This patchset fixes 2 issues with PMTU discovery that can lead to flood
> of retransmissions.
> The first patch fixes the issue for when PMTUD is disabled by the
> application, while the second fixes it for when its enabled.
>
> Please consider these to stable.
>
> Thanks,
>
> Marcelo Ricardo Leitner (2):
> sctp: do not retransmit upon FragNeeded if PMTU discovery is disabled
> sctp: fix the handling of ICMP Frag Needed for too small MTUs
>
> include/net/sctp/structs.h | 2 +-
> net/sctp/input.c | 28 ++++++++++++++++------------
> net/sctp/transport.c | 29 +++++++++++++++++++----------
> 3 files changed, 36 insertions(+), 23 deletions(-)
>
> --
> 2.14.3
>
Reviewed-by: Xin Long <lucien.xin@gmail.com>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 22/27] ixgbe: Use timecounter_reset interface
From: Brown, Aaron F @ 2018-01-06 5:11 UTC (permalink / raw)
To: Kamble, Sagar A, linux-kernel@vger.kernel.org
Cc: intel-wired-lan@lists.osuosl.org, Richard Cochran,
Kamble, Sagar A, netdev@vger.kernel.org
In-Reply-To: <1513323522-15021-23-git-send-email-sagar.a.kamble@intel.com>
> -----Original Message-----
> From: Brown, Aaron F
> Sent: Friday, January 5, 2018 8:34 PM
> To: 'Sagar Arun Kamble' <sagar.a.kamble@intel.com>; linux-
> kernel@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>; Kamble, Sagar A
> <sagar.a.kamble@intel.com>; netdev@vger.kernel.org
> Subject: RE: [Intel-wired-lan] [PATCH 22/27] ixgbe: Use timecounter_reset
> interface
>
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> > Behalf Of Sagar Arun Kamble
> > Sent: Thursday, December 14, 2017 11:39 PM
> > To: linux-kernel@vger.kernel.org
> > Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran
> > <richardcochran@gmail.com>; Kamble, Sagar A
> > <sagar.a.kamble@intel.com>; netdev@vger.kernel.org
> > Subject: [Intel-wired-lan] [PATCH 22/27] ixgbe: Use timecounter_reset
> > interface
> >
> > With new interface timecounter_reset we can update the start time for
> > timecounter. Update ixgbe_ptp_settime with this new function.
> >
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Cc: Richard Cochran <richardcochran@gmail.com>
> > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org
> > Cc: netdev@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Strike my Tested-by: for this (ixgbe) instance. It was meant for igb.
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH 09/27] igb: Use timecounter_initialize interface
From: Brown, Aaron F @ 2018-01-06 5:14 UTC (permalink / raw)
To: Kamble, Sagar A, linux-kernel@vger.kernel.org
Cc: intel-wired-lan@lists.osuosl.org, Richard Cochran,
Kamble, Sagar A, netdev@vger.kernel.org
In-Reply-To: <1513323522-15021-10-git-send-email-sagar.a.kamble@intel.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> Behalf Of Sagar Arun Kamble
> Sent: Thursday, December 14, 2017 11:38 PM
> To: linux-kernel@vger.kernel.org
> Cc: intel-wired-lan@lists.osuosl.org; Richard Cochran
> <richardcochran@gmail.com>; Kamble, Sagar A
> <sagar.a.kamble@intel.com>; netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH 09/27] igb: Use timecounter_initialize
> interface
>
> With new interface timecounter_initialize we can initialize timecounter
> fields and underlying cyclecounter together. Update igb ptp timecounter
> init with this new function.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/net/ethernet/intel/igb/igb.h | 4 ++++
> drivers/net/ethernet/intel/igb/igb_ptp.c | 23 ++++++++++++++---------
> 2 files changed, 18 insertions(+), 9 deletions(-)
>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ permalink raw reply
* Re: [PATCH 01/18] asm-generic/barrier: add generic nospec helpers
From: Dan Williams @ 2018-01-06 5:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, Mark Rutland, linux-arch,
Peter Zijlstra, Network Development, Will Deacon,
Greg Kroah-Hartman, Thomas Gleixner, Alan Cox
In-Reply-To: <CA+55aFxoU1B7VECQ3T9CRkX07Y8sw=fGg6Miry1GniqJOd68sA@mail.gmail.com>
On Fri, Jan 5, 2018 at 6:55 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> +#ifndef nospec_ptr
>> +#define nospec_ptr(ptr, lo, hi) \
>
> Do we actually want this horrible interface?
>
> It just causes the compiler - or inline asm - to generate worse code,
> because it needs to compare against both high and low limits.
>
> Basically all users are arrays that are zero-based, and where a
> comparison against the high _index_ limit would be sufficient.
>
> But the way this is all designed, it's literally designed for bad code
> generation for the unusual case, and the usual array case is written
> in the form of the unusual and wrong non-array case. That really seems
> excessively stupid.
Yes, it appears we can kill nospec_ptr() and move nospec_array_ptr()
to assume 0 based arrays rather than use nospec_ptr.
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Dan Williams @ 2018-01-06 5:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, linux-arch, Andi Kleen, Arnd Bergmann,
Greg Kroah-Hartman, Peter Zijlstra, Network Development,
the arch/x86 maintainers, Ingo Molnar, H. Peter Anvin,
Thomas Gleixner, Alan Cox
In-Reply-To: <CA+55aFzeCHgAtz4vCR9YaUxkuesCNEht56dKJmpytx2A-JmJkg@mail.gmail.com>
On Fri, Jan 5, 2018 at 6:52 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> When access_ok fails we should always stop speculating.
>> Add the required barriers to the x86 access_ok macro.
>
> Honestly, this seems completely bogus.
>
> The description is pure garbage afaik.
>
> The fact is, we have to stop speculating when access_ok() does *not*
> fail - because that's when we'll actually do the access. And it's that
> access that needs to be non-speculative.
>
> That actually seems to be what the code does (it stops speculation
> when __range_not_ok() returns false, but access_ok() is
> !__range_not_ok()). But the explanation is crap, and dangerous.
Oh, bother, yes, good catch. It's been a long week. I'll take a look
at moving this to uaccess_begin().
^ permalink raw reply
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
From: Pravin Shelar @ 2018-01-06 6:17 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
In-Reply-To: <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
> wrote:
>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> OVS already pull all required headers in skb linear data, so no need
>>> to redo all of it. only check required is the ip-checksum validation.
>>> I think we could avoid it in most of cases by checking skb length to
>>> ipheader length before verifying the ip header-checksum.
>>
>> Shouldn't the IP header checksum be verified even earlier, like in
>> key_extract(), before actually using any of the fields in the IP
>> header?
>
> Something like this for verifying the IP header checksum (not tested):
>
AFAIU openflow does not need this verification, so it is not required
in flow extract.
^ permalink raw reply
* Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing
From: Pravin Shelar @ 2018-01-06 6:17 UTC (permalink / raw)
To: Ed Swierk
Cc: ovs-dev, Linux Kernel Network Developers, Benjamin Warren,
Keith Holleman
In-Reply-To: <CAO_EM_mQgURXZNtW7Qw7OkW4rjp4JWKBmqS8e4pUR=ZuiGCcZQ@mail.gmail.com>
On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@skyportsystems.com>
> wrote:
>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> OVS already pull all required headers in skb linear data, so no need
>>> to redo all of it. only check required is the ip-checksum validation.
>>> I think we could avoid it in most of cases by checking skb length to
>>> ipheader length before verifying the ip header-checksum.
>>
>> Shouldn't the IP header checksum be verified even earlier, like in
>> key_extract(), before actually using any of the fields in the IP
>> header?
>
> Something like this for verifying the IP header checksum (not tested):
>
AFAIU openflow does not need this verification, so it is not required
in flow extract.
^ permalink raw reply
* Re: [PATCH net-next 06/20] net: hns3: Modify the update period of packet statistics
From: lipeng (Y) @ 2018-01-06 6:23 UTC (permalink / raw)
To: Andrew Lunn; +Cc: davem, netdev, linux-kernel, linuxarm, salil.mehta
In-Reply-To: <20180105145405.GC4038@lunn.ch>
On 2018/1/5 22:54, Andrew Lunn wrote:
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -1126,6 +1126,7 @@ static int hns3_nic_set_features(struct net_device *netdev,
>> {
>> struct hns3_nic_priv *priv = netdev_priv(netdev);
>> int queue_num = priv->ae_handle->kinfo.num_tqps;
>> + struct hnae3_handle *handle = priv->ae_handle;
>> struct hns3_enet_ring *ring;
>> unsigned int start;
>> unsigned int idx;
>> @@ -1134,6 +1135,8 @@ static int hns3_nic_set_features(struct net_device *netdev,
>> u64 tx_pkts = 0;
>> u64 rx_pkts = 0;
>>
>> + handle->ae_algo->ops->update_stats(handle, &netdev->stats);
>> +
>> for (idx = 0; idx < queue_num; idx++) {
>> /* fetch the tx stats */
>> ring = priv->ring_data[idx].ring;
> There is something odd going on with patch here. Notice how it says
> hns3_nic_set_features(). This is not the function being patched, it is
> actually the next one, hns3_nic_get_stats64(), which makes a lot more
> sense.
>
> Is it because the static void is on the previous line?
Yes, it is because the static void is on the previous line.
I can add one patch to fix the previous line , and this patch will
correct automatically.
do it need V2 patchset? or push a new patch after this patchset?
>
> It would be nice if the function was correctly reported. It makes it
> easier to review the patch.
>
> Andrew
>
> .
>
^ permalink raw reply
* Re: [PATCH 00/18] prevent bounds-check bypass via speculative execution
From: Dan Williams @ 2018-01-06 6:30 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linux Kernel Mailing List, Mark Rutland, Peter Zijlstra, Alan Cox,
Srinivas Pandruvada, Will Deacon, Solomon Peachy, H. Peter Anvin,
Christian Lamparter, Elena Reshetova,
linux-arch-u79uwXL29TY76Z2rM5mHXA, Andi Kleen,
James E.J. Bottomley, linux-scsi, Jonathan Corbet, X86 ML,
Ingo Molnar, Alexey Kuznetsov, Zhang Rui,
"Linux-media-u79uwXL29TY@public.gmane.org
In-Reply-To: <87y3lbpvzp.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On Fri, Jan 5, 2018 at 6:22 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
> Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
>
>> Quoting Mark's original RFC:
>>
>> "Recently, Google Project Zero discovered several classes of attack
>> against speculative execution. One of these, known as variant-1, allows
>> explicit bounds checks to be bypassed under speculation, providing an
>> arbitrary read gadget. Further details can be found on the GPZ blog [1]
>> and the Documentation patch in this series."
>>
>> This series incorporates Mark Rutland's latest api and adds the x86
>> specific implementation of nospec_barrier. The
>> nospec_{array_ptr,ptr,barrier} helpers are then combined with a kernel
>> wide analysis performed by Elena Reshetova to address static analysis
>> reports where speculative execution on a userspace controlled value
>> could bypass a bounds check. The patches address a precondition for the
>> attack discussed in the Spectre paper [2].
>
> Please expand this.
>
> It is not clear what the static analysis is looking for. Have a clear
> description of what is being fixed is crucial for allowing any of these
> changes.
>
> For the details given in the change description what I read is magic
> changes because a magic process says this code is vunlerable.
Yes, that was my first reaction to the patches as well, I try below to
add some more background and guidance, but in the end these are static
analysis reports across a wide swath of sub-systems. It's going to
take some iteration with domain experts to improve the patch
descriptions, and that's the point of this series, to get the better
trained eyes from the actual sub-system owners to take a look at these
reports.
For example, I'm looking for feedback like what Srinivas gave where he
identified that the report is bogus, the branch condition can not be
seeded with bad values in that path. Be like Srinivas.
> Given the similarities in the code that is being patched to many other
> places in the kernel it is not at all clear that this small set of
> changes is sufficient for any purpose.
I find this assertion absurd, when in the past have we as kernel
developers ever been handed a static analysis report and then
questioned why the static analysis did not flag other call sites
before first reviewing the ones it did find?
>> A consideration worth noting for reviewing these patches is to weigh the
>> dramatic cost of being wrong about whether a given report is exploitable
>> vs the overhead nospec_{array_ptr,ptr} may introduce. In other words,
>> lets make the bar for applying these patches be "can you prove that the
>> bounds check bypass is *not* exploitable". Consider that the Spectre
>> paper reports one example of a speculation window being ~180 cycles.
>
>
>> Note that there is also a proposal from Linus, array_access [3], that
>> attempts to quash speculative execution past a bounds check without
>> introducing an lfence instruction. That may be a future optimization
>> possibility that is compatible with this api, but it would appear to
>> need guarantees from the compiler that it is not clear the kernel can
>> rely on at this point. It is also not clear that it would be a
>> significant performance win vs lfence.
>
> It is also not clear that these changes fix anything, or are in any
> sense correct for the problem they are trying to fix as the problem
> is not clearly described.
I'll try my best. I don't have first hand knowledge of how the static
analyzer is doing this job, and I don't think it matters for
evaluating these reports. I'll give you my thoughts on how I would
handle one of these reports if it flagged one of the sub-systems I
maintain.
Start with the example from the Spectre paper:
if (x < array1_size)
y = array2[array1[x] * 256];
In all the patches 'x' and 'array1' are called out explicitly. For example:
net: mpls: prevent bounds-check bypass via speculative execution
Static analysis reports that 'index' may be a user controlled value that
is used as a data dependency reading 'rt' from the 'platform_label'
array...
So the first thing to review is whether the analyzer got it wrong and
'x' is not arbitrarily controllable by userspace to cause speculation
outside of the checked bounds. Be like Srinivas. The next step is to
ask whether the code can be refactored so that 'x' is sanitized
earlier in the call stack, especially if the nospec_array_ptr() lands
in a hot path. The next aspect that I expect most would be tempted to
go check is whether 'array2[array1[x]]' occurs later in the code
stream, but with speculation windows being architecture dependent and
potentially large (~180 cycles in one case says the paper) I submit
that we should err on the side of caution and not guess if that second
dependent read has been emitted somewhere in the instruction stream.
> In at least one place (mpls) you are patching a fast path. Compile out
> or don't load mpls by all means. But it is not acceptable to change the
> fast path without even considering performance.
Performance matters greatly, but I need help to identify a workload
that is representative for this fast path to see what, if any, impact
is incurred. Even better is a review that says "nope, 'index' is not
subject to arbitrary userspace control at this point, drop the patch."
^ permalink raw reply
* Re: KASAN: use-after-free Read in sctp_packet_transmit
From: Xin Long @ 2018-01-06 6:50 UTC (permalink / raw)
To: syzbot
Cc: davem, LKML, linux-sctp, network dev, Neil Horman, syzkaller-bugs,
Vlad Yasevich
In-Reply-To: <94eb2c1fcf4cf899b405620eaa66@google.com>
On Sat, Jan 6, 2018 at 6:07 AM, syzbot
<syzbot+5adcca18fca253b4cb15@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzkaller hit the following crash on
> 8a4816cad00bf14642f0ed6043b32d29a05006ce
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> Unfortunately, I don't have any reproducer for this bug yet.
>
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5adcca18fca253b4cb15@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> ==================================================================
> BUG: KASAN: use-after-free in sctp_packet_transmit+0x3505/0x3750
> net/sctp/output.c:643
> Read of size 8 at addr ffff8801bda9fb80 by task modprobe/23740
>
> CPU: 1 PID: 23740 Comm: modprobe Not tainted 4.15.0-rc5+ #175
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x257 lib/dump_stack.c:53
> print_address_description+0x73/0x250 mm/kasan/report.c:252
> kasan_report_error mm/kasan/report.c:351 [inline]
> kasan_report+0x25b/0x340 mm/kasan/report.c:409
> __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:430
> sctp_packet_transmit+0x3505/0x3750 net/sctp/output.c:643
> sctp_outq_flush+0x121b/0x4060 net/sctp/outqueue.c:1197
> sctp_outq_uncork+0x5a/0x70 net/sctp/outqueue.c:776
> sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1807 [inline]
> sctp_side_effects net/sctp/sm_sideeffect.c:1210 [inline]
> sctp_do_sm+0x4e0/0x6ed0 net/sctp/sm_sideeffect.c:1181
> sctp_generate_heartbeat_event+0x292/0x3f0 net/sctp/sm_sideeffect.c:406
> call_timer_fn+0x228/0x820 kernel/time/timer.c:1320
> expire_timers kernel/time/timer.c:1357 [inline]
> __run_timers+0x7ee/0xb70 kernel/time/timer.c:1660
> run_timer_softirq+0x4c/0xb0 kernel/time/timer.c:1686
> __do_softirq+0x2d7/0xb85 kernel/softirq.c:285
> invoke_softirq kernel/softirq.c:365 [inline]
> irq_exit+0x1cc/0x200 kernel/softirq.c:405
> exiting_irq arch/x86/include/asm/apic.h:540 [inline]
> smp_apic_timer_interrupt+0x16b/0x700 arch/x86/kernel/apic/apic.c:1052
> apic_timer_interrupt+0xa9/0xb0 arch/x86/entry/entry_64.S:904
> </IRQ>
> RIP: 0010:__preempt_count_add arch/x86/include/asm/preempt.h:76 [inline]
> RIP: 0010:__rcu_read_lock include/linux/rcupdate.h:83 [inline]
> RIP: 0010:rcu_read_lock include/linux/rcupdate.h:629 [inline]
> RIP: 0010:__is_insn_slot_addr+0x8f/0x330 kernel/kprobes.c:303
> RSP: 0018:ffff8801d4937430 EFLAGS: 00000283 ORIG_RAX: ffffffffffffff11
> RAX: ffff8801bf13c000 RBX: ffffffff8656dd00 RCX: ffffffff8170bd88
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff8656dd00
> RBP: ffff8801d4937518 R08: 0000000000000000 R09: 1ffff1003a926e67
> R10: ffff8801d4937300 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000000 R14: ffff8801d49374f0 R15: ffff8801dae230c0
> is_kprobe_insn_slot include/linux/kprobes.h:318 [inline]
> kernel_text_address+0x132/0x140 kernel/extable.c:150
> __kernel_text_address+0xd/0x40 kernel/extable.c:107
> unwind_get_return_address+0x61/0xa0 arch/x86/kernel/unwind_frame.c:18
> __save_stack_trace+0x7e/0xd0 arch/x86/kernel/stacktrace.c:45
> save_stack_trace+0x1a/0x20 arch/x86/kernel/stacktrace.c:60
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
> kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:489
> kmem_cache_alloc+0x12e/0x760 mm/slab.c:3544
> kmem_cache_zalloc include/linux/slab.h:678 [inline]
> file_alloc_security security/selinux/hooks.c:369 [inline]
> selinux_file_alloc_security+0xae/0x190 security/selinux/hooks.c:3454
> security_file_alloc+0x6d/0xa0 security/security.c:873
> get_empty_filp+0x189/0x4f0 fs/file_table.c:129
> path_openat+0xed/0x3530 fs/namei.c:3496
> do_filp_open+0x25b/0x3b0 fs/namei.c:3554
> do_sys_open+0x502/0x6d0 fs/open.c:1059
> SYSC_open fs/open.c:1077 [inline]
> SyS_open+0x2d/0x40 fs/open.c:1072
> entry_SYSCALL_64_fastpath+0x23/0x9a
> RIP: 0033:0x7efdff1bb120
> RSP: 002b:00007ffde6213c08 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
> RAX: ffffffffffffffda RBX: 000055c34fab4090 RCX: 00007efdff1bb120
> RDX: 00000000000001b6 RSI: 0000000000080000 RDI: 00007ffde6213d20
> RBP: 00007ffde6214d90 R08: 0000000000000008 R09: 0000000000000001
> R10: 0000000000000000 R11: 0000000000000246 R12: 000055c34fab4090
> R13: 00007ffde6215de0 R14: 0000000000000000 R15: 0000000000000000
>
> Allocated by task 23739:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
> kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:489
> kmem_cache_alloc+0x12e/0x760 mm/slab.c:3544
> kmem_cache_zalloc include/linux/slab.h:678 [inline]
> sctp_chunkify+0xce/0x3f0 net/sctp/sm_make_chunk.c:1329
> _sctp_make_chunk+0x13c/0x260 net/sctp/sm_make_chunk.c:1397
> sctp_make_control+0x39/0x150 net/sctp/sm_make_chunk.c:1433
> sctp_make_heartbeat+0x90/0x420 net/sctp/sm_make_chunk.c:1151
> sctp_sf_heartbeat.isra.24+0x26/0x180 net/sctp/sm_statefuns.c:973
> sctp_sf_do_prm_requestheartbeat+0x27/0x100 net/sctp/sm_statefuns.c:5251
> sctp_do_sm+0x192/0x6ed0 net/sctp/sm_sideeffect.c:1178
> sctp_primitive_REQUESTHEARTBEAT+0xa0/0xd0 net/sctp/primitive.c:200
> sctp_apply_peer_addr_params+0x759/0xf30 net/sctp/socket.c:2462
> sctp_setsockopt_peer_addr_params+0x36f/0x5f0 net/sctp/socket.c:2658
> sctp_setsockopt+0x199a/0x61a0 net/sctp/socket.c:4173
> sock_common_setsockopt+0x95/0xd0 net/core/sock.c:2978
> SYSC_setsockopt net/socket.c:1821 [inline]
> SyS_setsockopt+0x189/0x360 net/socket.c:1800
> entry_SYSCALL_64_fastpath+0x23/0x9a
>
> Freed by task 23739:
> save_stack+0x43/0xd0 mm/kasan/kasan.c:447
> set_track mm/kasan/kasan.c:459 [inline]
> kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
> __cache_free mm/slab.c:3488 [inline]
> kmem_cache_free+0x83/0x2a0 mm/slab.c:3746
> sctp_chunk_destroy net/sctp/sm_make_chunk.c:1450 [inline]
> sctp_chunk_put+0x2fd/0x420 net/sctp/sm_make_chunk.c:1473
> sctp_chunk_free+0x53/0x60 net/sctp/sm_make_chunk.c:1460
> sctp_packet_transmit+0xf5d/0x3750 net/sctp/output.c:646
> sctp_outq_flush+0x121b/0x4060 net/sctp/outqueue.c:1197
> sctp_outq_uncork+0x5a/0x70 net/sctp/outqueue.c:776
> sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1807 [inline]
> sctp_side_effects net/sctp/sm_sideeffect.c:1210 [inline]
> sctp_do_sm+0x4e0/0x6ed0 net/sctp/sm_sideeffect.c:1181
> sctp_primitive_REQUESTHEARTBEAT+0xa0/0xd0 net/sctp/primitive.c:200
> sctp_apply_peer_addr_params+0x759/0xf30 net/sctp/socket.c:2462
> sctp_setsockopt_peer_addr_params+0x36f/0x5f0 net/sctp/socket.c:2658
> sctp_setsockopt+0x199a/0x61a0 net/sctp/socket.c:4173
> sock_common_setsockopt+0x95/0xd0 net/core/sock.c:2978
> SYSC_setsockopt net/socket.c:1821 [inline]
> SyS_setsockopt+0x189/0x360 net/socket.c:1800
> entry_SYSCALL_64_fastpath+0x23/0x9a
>
> The buggy address belongs to the object at ffff8801bda9fb80
> which belongs to the cache sctp_chunk of size 256
> The buggy address is located 0 bytes inside of
> 256-byte region [ffff8801bda9fb80, ffff8801bda9fc80)
> The buggy address belongs to the page:
> page:00000000d1261812 count:1 mapcount:0 mapping:000000003e733284 index:0x0
> flags: 0x2fffc0000000100(slab)
> raw: 02fffc0000000100 ffff8801bda9f040 0000000000000000 000000010000000c
> raw: ffffea000714c9e0 ffffea0006fa8520 ffff8801d3246c80 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8801bda9fa80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8801bda9fb00: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>>
>> ffff8801bda9fb80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>
> ^
> ffff8801bda9fc00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8801bda9fc80: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
> ==================================================================
Couldn't see how this happened, but I noticed one buggy thing:
when peeling off an assoc, it doesn't reset the sk owner for the
chunks in q->control_chunk_list. It causes these skbs to have
the incorrect skb->sk. This might be relevant.
>
>
> ---
> This bug is generated by a dumb bot. It may contain errors.
> See https://goo.gl/tpsmEJ for details.
> Direct all questions to syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report.
> If you forgot to add the Reported-by tag, once the fix for this bug is
> merged
> into any tree, please reply to this email with:
> #syz fix: exact-commit-title
> To mark this as a duplicate of another syzbot report, please reply with:
> #syz dup: exact-subject-of-another-report
> If it's a one-off invalid bug report, please reply with:
> #syz invalid
> Note: if the crash happens again, it will cause creation of a new bug
> report.
> Note: all commands must start from beginning of the line in the email body.
^ permalink raw reply
* Re: [wireless-testsing2:master 1/4] drivers/net/netdevsim/bpf.c:130:14: sparse: incompatible types for 'case' statement
From: Fengguang Wu @ 2018-01-06 7:04 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David S. Miller, kbuild-all, netdev, Bob Copeland
In-Reply-To: <20180103170237.0d682aa0@cakuba.netronome.com>
On Wed, Jan 03, 2018 at 05:02:37PM -0800, Jakub Kicinski wrote:
>On Thu, 4 Jan 2018 03:53:20 +0800, kbuild test robot wrote:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-testing.git master
>> head: 6b3b30d0c31ddb2f4d8208c90bc2b4adef47204d
>> commit: af2cae39f6ab9dc596616d6a28c7772e1dd55e91 [1/4] Merge remote-tracking branch 'wireless-drivers-next/master'
>> reproduce:
>> # apt-get install sparse
>> git checkout af2cae39f6ab9dc596616d6a28c7772e1dd55e91
>> make ARCH=x86_64 allmodconfig
>> make C=1 CF=-D__CHECK_ENDIAN__
>
>> drivers/net/netdevsim/bpf.c: In function 'nsim_bpf_setup_tc_block_cb':
>> >> drivers/net/netdevsim/bpf.c:130:7: error: 'TC_CLSBPF_REPLACE' undeclared (first use in this function); did you mean 'TC_RED_REPLACE'?
>> case TC_CLSBPF_REPLACE:
>> ^~~~~~~~~~~~~~~~~
>> TC_RED_REPLACE
>
>FWIW looks like the tree contains old net-next code and latest net
>(linux/master) code. Pulling from net-next will solve this.
>
>> :::::: TO: Jakub Kicinski <jakub.kicinski@netronome.com>
>> :::::: CC: Daniel Borkmann <daniel@iogearbox.net>
>
>Interestingly Daniel and I were not CCed on the report, is this
>intentional?
Yeah the above ":::::: TO/CC" lines are for manual addition when
appropriate. They are the author/committer of the commit that last
modified the code line in question.
Thanks,
Fengguang
^ permalink raw reply
* Re: [PATCH net] ipv6: remove null_entry before adding default route
From: Martin KaFai Lau @ 2018-01-06 7:42 UTC (permalink / raw)
To: Wei Wang; +Cc: David Miller, netdev, Eric Dumazet
In-Reply-To: <20180106013835.127556-1-tracywwnj@gmail.com>
On Fri, Jan 05, 2018 at 05:38:35PM -0800, Wei Wang wrote:
> From: Wei Wang <weiwan@google.com>
>
> In the current code, when creating a new fib6 table, tb6_root.leaf gets
> initialized to net->ipv6.ip6_null_entry.
> If a default route is being added with rt->rt6i_metric = 0xffffffff,
> fib6_add() will add this route after net->ipv6.ip6_null_entry. As
> null_entry is shared, it could cause problem.
>
> In order to fix it, set fn->leaf to NULL before calling
> fib6_add_rt2node() when trying to add the first default route.
> And reset fn->leaf to null_entry when adding fails or when deleting the
> last default route.
>
> syzkaller reported the following issue which is fixed by this commit:
> =============================
> WARNING: suspicious RCU usage
> 4.15.0-rc5+ #171 Not tainted
> -----------------------------
> net/ipv6/ip6_fib.c:1702 suspicious rcu_dereference_protected() usage!
>
> other info that might help us debug this:
>
> rcu_scheduler_active = 2, debug_locks = 1
> 4 locks held by swapper/0/0:
> #0: ((&net->ipv6.ip6_fib_timer)){+.-.}, at: [<00000000d43f631b>] lockdep_copy_map include/linux/lockdep.h:178 [inline]
> #0: ((&net->ipv6.ip6_fib_timer)){+.-.}, at: [<00000000d43f631b>] call_timer_fn+0x1c6/0x820 kernel/time/timer.c:1310
> #1: (&(&net->ipv6.fib6_gc_lock)->rlock){+.-.}, at: [<000000002ff9d65c>] spin_lock_bh include/linux/spinlock.h:315 [inline]
> #1: (&(&net->ipv6.fib6_gc_lock)->rlock){+.-.}, at: [<000000002ff9d65c>] fib6_run_gc+0x9d/0x3c0 net/ipv6/ip6_fib.c:2007
> #2: (rcu_read_lock){....}, at: [<0000000091db762d>] __fib6_clean_all+0x0/0x3a0 net/ipv6/ip6_fib.c:1560
> #3: (&(&tb->tb6_lock)->rlock){+.-.}, at: [<000000009e503581>] spin_lock_bh include/linux/spinlock.h:315 [inline]
> #3: (&(&tb->tb6_lock)->rlock){+.-.}, at: [<000000009e503581>] __fib6_clean_all+0x1d0/0x3a0 net/ipv6/ip6_fib.c:1948
>
> stack backtrace:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.15.0-rc5+ #171
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:17 [inline]
> dump_stack+0x194/0x257 lib/dump_stack.c:53
> lockdep_rcu_suspicious+0x123/0x170 kernel/locking/lockdep.c:4585
> fib6_del+0xcaa/0x11b0 net/ipv6/ip6_fib.c:1701
> fib6_clean_node+0x3aa/0x4f0 net/ipv6/ip6_fib.c:1892
> fib6_walk_continue+0x46c/0x8a0 net/ipv6/ip6_fib.c:1815
> fib6_walk+0x91/0xf0 net/ipv6/ip6_fib.c:1863
> fib6_clean_tree+0x1e6/0x340 net/ipv6/ip6_fib.c:1933
> __fib6_clean_all+0x1f4/0x3a0 net/ipv6/ip6_fib.c:1949
> fib6_clean_all net/ipv6/ip6_fib.c:1960 [inline]
> fib6_run_gc+0x16b/0x3c0 net/ipv6/ip6_fib.c:2016
> fib6_gc_timer_cb+0x20/0x30 net/ipv6/ip6_fib.c:2033
> call_timer_fn+0x228/0x820 kernel/time/timer.c:1320
> expire_timers kernel/time/timer.c:1357 [inline]
> __run_timers+0x7ee/0xb70 kernel/time/timer.c:1660
> run_timer_softirq+0x4c/0xb0 kernel/time/timer.c:1686
> __do_softirq+0x2d7/0xb85 kernel/softirq.c:285
> invoke_softirq kernel/softirq.c:365 [inline]
> irq_exit+0x1cc/0x200 kernel/softirq.c:405
> exiting_irq arch/x86/include/asm/apic.h:540 [inline]
> smp_apic_timer_interrupt+0x16b/0x700 arch/x86/kernel/apic/apic.c:1052
> apic_timer_interrupt+0xa9/0xb0 arch/x86/entry/entry_64.S:904
> </IRQ>
>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: 66f5d6ce53e6 ("ipv6: replace rwlock with rcu and spinlock in fib6_table")
> Signed-off-by: Wei Wang <weiwan@google.com>
> ---
> net/ipv6/ip6_fib.c | 45 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 35 insertions(+), 10 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index d11a5578e4f8..37cb4ad1ea29 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -640,6 +640,11 @@ static struct fib6_node *fib6_add_1(struct net *net,
> if (!(fn->fn_flags & RTN_RTINFO)) {
> RCU_INIT_POINTER(fn->leaf, NULL);
> rt6_release(leaf);
> + /* remove null_entry in the root node */
> + } else if (fn->fn_flags & RTN_TL_ROOT &&
> + rcu_access_pointer(fn->leaf) ==
> + net->ipv6.ip6_null_entry) {
> + RCU_INIT_POINTER(fn->leaf, NULL);
It seems the reader side could see tb6_root.leaf == NULL after
this change and I think it should be fine? If it is, instead
of switching betwen NULL and ip6_null_entry, would it be simpler
to always set tb6_root.leaf to NULL until a legit default route
is added?
> }
>
> return fn;
> @@ -1270,14 +1275,27 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt,
> return err;
>
> failure:
> - /* fn->leaf could be NULL if fn is an intermediate node and we
> - * failed to add the new route to it in both subtree creation
> - * failure and fib6_add_rt2node() failure case.
> - * In both cases, fib6_repair_tree() should be called to fix
> + /* fn->leaf could be NULL if:
> + * 1. fn is the root node in the table and we fail to add the default
> + * route to it.
> + * In this case, we put fn->leaf back to net->ipv6.ip6_null_entry as
> + * the way the table was created.
> + * 2. fn is an intermediate node and we failed to add the new
> + * route to it in both subtree creation failure and fib6_add_rt2node()
> + * failure case.
> + * In this case, fib6_repair_tree() should be called to fix
> * fn->leaf.
> */
> - if (fn && !(fn->fn_flags & (RTN_RTINFO|RTN_ROOT)))
> - fib6_repair_tree(info->nl_net, table, fn);
> + if (fn) {
> + if (fn->fn_flags & RTN_TL_ROOT) {
> + if (!rcu_access_pointer(fn->leaf)) {
> + rcu_assign_pointer(fn->leaf,
> + info->nl_net->ipv6.ip6_null_entry);
> + }
> + } else if (!(fn->fn_flags & (RTN_RTINFO|RTN_ROOT))) {
> + fib6_repair_tree(info->nl_net, table, fn);
> + }
> + }
> /* Always release dst as dst->__refcnt is guaranteed
> * to be taken before entering this function
> */
> @@ -1685,11 +1703,18 @@ static void fib6_del_route(struct fib6_table *table, struct fib6_node *fn,
> }
> read_unlock(&net->ipv6.fib6_walker_lock);
>
> - /* If it was last route, expunge its radix tree node */
> + /* If it was last route:
> + * 1. For root node, put back null_entry as how the table was created.
> + * 2. For other nodes, expunge its radix tree node.
> + */
> if (!rcu_access_pointer(fn->leaf)) {
> - fn->fn_flags &= ~RTN_RTINFO;
> - net->ipv6.rt6_stats->fib_route_nodes--;
> - fn = fib6_repair_tree(net, table, fn);
> + if (fn->fn_flags & RTN_TL_ROOT) {
> + rcu_assign_pointer(fn->leaf, net->ipv6.ip6_null_entry);
> + } else {
> + fn->fn_flags &= ~RTN_RTINFO;
> + net->ipv6.rt6_stats->fib_route_nodes--;
> + fn = fib6_repair_tree(net, table, fn);
> + }
> }
>
> fib6_purge_rt(rt, fn, net);
> --
> 2.16.0.rc0.223.g4a4ac83678-goog
>
^ permalink raw reply
* [PATCH] mdio-sun4i: Fix a memory leak
From: Christophe JAILLET @ 2018-01-06 8:00 UTC (permalink / raw)
To: andrew, f.fainelli, maxime.ripard, wens
Cc: netdev, linux-arm-kernel, linux-kernel, kernel-janitors,
Christophe JAILLET
If the probing of the regulator is deferred, the memory allocated by
'mdiobus_alloc_size()' will be leaking.
It should be freed before the next call to 'sun4i_mdio_probe()' which will
reallocate it.
Fixes: 4bdcb1dd9feb ("net: Add MDIO bus driver for the Allwinner EMAC")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/net/phy/mdio-sun4i.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mdio-sun4i.c b/drivers/net/phy/mdio-sun4i.c
index 135296508a7e..6425ce04d3f9 100644
--- a/drivers/net/phy/mdio-sun4i.c
+++ b/drivers/net/phy/mdio-sun4i.c
@@ -118,8 +118,10 @@ static int sun4i_mdio_probe(struct platform_device *pdev)
data->regulator = devm_regulator_get(&pdev->dev, "phy");
if (IS_ERR(data->regulator)) {
- if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
- return -EPROBE_DEFER;
+ if (PTR_ERR(data->regulator) == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto err_out_free_mdiobus;
+ }
dev_info(&pdev->dev, "no regulator found\n");
data->regulator = NULL;
--
2.14.1
^ permalink raw reply related
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: Jiri Pirko @ 2018-01-06 8:07 UTC (permalink / raw)
To: David Ahern
Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <a62e5912-8476-00aa-a099-b2f63da2c563@gmail.com>
Sat, Jan 06, 2018 at 04:57:21AM CET, dsahern@gmail.com wrote:
>On 1/5/18 4:09 PM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Currently the filters added to qdiscs are independent. So for example if you
>> have 2 netdevices and you create ingress qdisc on both and you want to add
>> identical filter rules both, you need to add them twice. This patchset
>> makes this easier and mainly saves resources allowing to share all filters
>> within a qdisc - I call it a "filter block". Also this helps to save
>> resources when we do offload to hw for example to expensive TCAM.
>>
>> So back to the example. First, we create 2 qdiscs. Both will share
>> block number 22. "22" is just an identification. If we don't pass any
>> block number, a new one will be generated by kernel:
>>
>> $ tc qdisc add dev ens7 ingress block 22
>> ^^^^^^^^
>> $ tc qdisc add dev ens8 ingress block 22
>> ^^^^^^^^
>>
>> Now if we list the qdiscs, we will see the block index in the output:
>>
>> $ tc qdisc
>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
>> qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
>>
>>
>> To make is more visual, the situation looks like this:
>>
>> ens7 ingress qdisc ens7 ingress qdisc
>> | |
>> | |
>> +----------> block 22 <----------+
>>
>> Unlimited number of qdiscs may share the same block.
>>
>> Now we can add filter using the block index:
>>
>> $ tc filter add block 22 protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>
>>
>> Note we cannot use the qdisc for filter manipulations for shared blocks:
>>
>> $ tc filter add dev ens8 ingress protocol ip pref 1 flower dst_ip 192.168.100.2 action drop
>> Error: Cannot work with shared block, please use block index.
>>
>>
>> We will see the same output if we list filters for ingress qdisc of
>> ens7 and ens8, also for the block 22:
>>
>> $ tc filter show block 22
>> filter block 22 protocol ip pref 25 flower chain 0
>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>> ...
>>
>> $ tc filter show dev ens7 ingress
>> filter block 22 protocol ip pref 25 flower chain 0
>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>> ...
>>
>> $ tc filter show dev ens8 ingress
>> filter block 22 protocol ip pref 25 flower chain 0
>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>> ...
>
>I like the API and output shown here, but I am not getting that with the
>patches.
>
>In this example, I am using 42 for the block id:
>
>$ tc qdisc show dev eth2
>qdisc mq 0: root
>qdisc pfifo_fast 0: parent :2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>1 1 1
>qdisc pfifo_fast 0: parent :1 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>1 1 1
>qdisc ingress ffff: parent ffff:fff1 block 42
>
>It allows me to add a filter using the device:
>$ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
>192.168.101.2 action drop
>$ echo $?
>0
Yes, because the block is not shared yet. You have it only for one
qdisc. As long as you have that, the "filter add dev" api still works.
It stops working when you add another qdisc to that block.
>
>And it modifies the shared block:
>$ tc filter show block 42
>filter pref 1 flower chain 0
>filter pref 1 flower chain 0 handle 0x1
> eth_type ipv4
> dst_ip 192.168.100.2
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 2 ref 1 bind 1
>
>filter pref 1 flower chain 0 handle 0x2
> eth_type ipv4
> dst_ip 192.168.101.2
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 3 ref 1 bind 1
>
>filter pref 25 flower chain 0
>filter pref 25 flower chain 0 handle 0x1
> eth_type ipv4
> dst_ip 192.168.0.0/16
> not_in_hw
> action order 1: gact action drop
> random type none pass val 0
> index 1 ref 1 bind 1
>
>Notice the header does not give the 'filter block N protocol' part. I
>don't get that using the device either (tc filter show dev eth2 ingress).
That is correct. Check the print_filter function in tc/tc_filter.c. It
works with "filter_ifindex" and with my patch with "filter_block_index".
That means that if the value for the filter dumped actually differs from
what you passed on the command line, it prints it.
Once you actually share the block with another qdisc, you will see
"block N"
>
>Something else I noticed is that I do not get an error message if I pass
>an invalid block id:
>
>$ tc filter show block 22
>$ echo $?
>0
>$ tc qdisc show | grep block
>qdisc ingress ffff: dev eth2 parent ffff:fff1 block 42
Yeah, I will try to fix this. The thing is, this is not error by kernel
but by the userspace. Kernel is perfectly ok with invalid device or
block index, it just does not dump anything and I would leave it like
that. I have to somehow check the validity of block_index in userspace.
Not sure how now.
Thanks!
^ permalink raw reply
* Re: [PATCH 14/18] ipv4: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:00 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, Hideaki YOSHIFUJI, netdev, peterz,
Alexey Kuznetsov, tglx, torvalds, David S. Miller,
Elena Reshetova, alan
In-Reply-To: <151520107001.32271.12149241186695668220.stgit@dwillia2-desk3.amr.corp.intel.com>
On Fri, Jan 05, 2018 at 05:11:10PM -0800, Dan Williams wrote:
> Static analysis reports that 'offset' may be a user controlled value
> that is used as a data dependency reading from a raw_frag_vec buffer.
> In order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue further
> reads based on an invalid '*(rfv->c + offset)' value.
>
> Based on an original patch by Elena Reshetova.
I thought we "proved" that this patch was not needed at all, based on
previous review. It doesn't look like that review cycle got
incorporated into this patch series at all, I guess I have to go back
and do it all again :(
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 14/18] ipv4: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:01 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, Hideaki YOSHIFUJI, netdev, peterz,
Alexey Kuznetsov, tglx, torvalds, David S. Miller,
Elena Reshetova, alan
In-Reply-To: <151520107001.32271.12149241186695668220.stgit@dwillia2-desk3.amr.corp.intel.com>
On Fri, Jan 05, 2018 at 05:11:10PM -0800, Dan Williams wrote:
> Static analysis reports that 'offset' may be a user controlled value
Can I see the rule that determined that? It does not feel like that is
correct, given the 3+ levels deep that this function gets this value
from...
Same for the ipv6 patch, it's the same code logic.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:03 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, peterz, netdev, qla2xxx-upstream,
tglx, torvalds, Elena Reshetova, alan
In-Reply-To: <151520104838.32271.7038801336240727574.stgit@dwillia2-desk3.amr.corp.intel.com>
On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> Static analysis reports that 'handle' may be a user controlled value
> that is used as a data dependency to read 'sp' from the
> 'req->outstanding_cmds' array. In order to avoid potential leaks of
> kernel memory values, block speculative execution of the instruction
> stream that could issue reads based on an invalid value of 'sp'. In this
> case 'sp' is directly dereferenced later in the function.
I'm pretty sure that 'handle' comes from the hardware, not from
userspace, from what I can tell here. If we want to start auditing
__iomem data sources, great! But that's a bigger task, and one I don't
think we are ready to tackle...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:09 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, alan, peterz, netdev, Laurent Pinchart,
tglx, Mauro Carvalho Chehab, torvalds, Elena Reshetova,
linux-media
In-Reply-To: <151520103240.32271.14706852449205864676.stgit@dwillia2-desk3.amr.corp.intel.com>
On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> Static analysis reports that 'index' may be a user controlled value that
> is used as a data dependency to read 'pin' from the
> 'selector->baSourceID' array. In order to avoid potential leaks of
> kernel memory values, block speculative execution of the instruction
> stream that could issue reads based on an invalid value of 'pin'.
>
> Based on an original patch by Elena Reshetova.
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 3e7e283a44a8..7442626dc20e 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -22,6 +22,7 @@
> #include <linux/mm.h>
> #include <linux/wait.h>
> #include <linux/atomic.h>
> +#include <linux/compiler.h>
>
> #include <media/v4l2-common.h>
> #include <media/v4l2-ctrls.h>
> @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> struct uvc_entity *iterm = NULL;
> u32 index = input->index;
> int pin = 0;
> + __u8 *elem;
>
> if (selector == NULL ||
> (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> break;
> }
> pin = iterm->id;
> - } else if (index < selector->bNrInPins) {
> - pin = selector->baSourceID[index];
> + } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> + selector->bNrInPins))) {
> + pin = *elem;
I dug through this before, and I couldn't find where index came from
userspace, I think seeing the coverity rule would be nice.
And if this value really is user controlled, then why is this the only
v4l driver that is affected? This is a common callback.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/2] SolutionEngine771x: add Ether TSU resource
From: Sergei Shtylyov @ 2018-01-06 9:24 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, linux-sh; +Cc: netdev
In-Reply-To: <20180103201137.255228853@cogentembedded.com>
Hello!
On 1/3/2018 11:08 PM, Sergei Shtylyov wrote:
> After the Ether platform data is fixed, the driver probe() method would
> still fail since the 'struct sh_eth_cpu_data' corresponding to SH771x
> indicates the presence of TSU but the memory resource for it is absent.
> Add the missing TSU resource to both Ether devices and fix the harmless
> off-by-one error in the main memory resources, while at it...
>
> Fixes: 4986b996882d ("net: sh_eth: remove the SH_TSU_ADDR")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> arch/sh/boards/mach-se/770x/setup.c | 14 ++++++++++++--
> arch/sh/include/mach-se/mach/se.h | 1 +
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> Index: linux/arch/sh/boards/mach-se/770x/setup.c
> ===================================================================
> --- linux.orig/arch/sh/boards/mach-se/770x/setup.c
> +++ linux/arch/sh/boards/mach-se/770x/setup.c
> @@ -123,10 +123,15 @@ static struct sh_eth_plat_data sh_eth_pl
> static struct resource sh_eth0_resources[] = {
> [0] = {
> .start = SH_ETH0_BASE,
> - .end = SH_ETH0_BASE + 0x1B8,
> + .end = SH_ETH0_BASE + 0x1B8 - 1,
> .flags = IORESOURCE_MEM,
> },
> [1] = {
> + .start = SH_TSU_BASE,
> + .end = SH_TSU_BASE + 0xA00 - 1,
Oops, should be 0x200, not 0xA00.
> + .flags = IORESOURCE_MEM,
> + },
> + [2] = {
> .start = SH_ETH0_IRQ,
> .end = SH_ETH0_IRQ,
> .flags = IORESOURCE_IRQ,
> @@ -146,10 +151,15 @@ static struct platform_device sh_eth0_de
> static struct resource sh_eth1_resources[] = {
> [0] = {
> .start = SH_ETH1_BASE,
> - .end = SH_ETH1_BASE + 0x1B8,
> + .end = SH_ETH1_BASE + 0x1B8 - 1,
> .flags = IORESOURCE_MEM,
> },
> [1] = {
> + .start = SH_TSU_BASE,
> + .end = SH_TSU_BASE + 0xA00 - 1,
Same here. I''ll repost...
> + .flags = IORESOURCE_MEM,
> + },
> + [2] = {
> .start = SH_ETH1_IRQ,
MBR, Sergei
^ permalink raw reply
* [PATCH 3.2 01/06] "bridge should send gratuitous ARP to notify peer while a bond, which is a port of this bridge, changes."
From: 邢庆杰 @ 2018-01-06 9:25 UTC (permalink / raw)
To: stephen, davem; +Cc: netdev, bridge, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1626 bytes --]
We create bond0 and add eth0ð1 as slaves. Eth0 is active. Then we add
bond0
into br0 as a bridge port. Br0 has ip address. When eth0 is down, after
bond0's
failover eth1 become active. At this moment, we need br0 send a gratuitous
ARP
to notify peer to update ARP table.
Signed-off-by: Xing.Qingjie <xqjcool@gmail.com>
---
net/bridge/br_notify.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index a76b621..90cf123 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c
@@ -34,6 +34,7 @@ static int br_device_event(struct notifier_block *unused,
unsigned long event, v
struct net_device *dev = ptr;
struct net_bridge_port *p;
struct net_bridge *br;
+ struct in_device *in_dev;
bool changed_addr;
int err;
@@ -102,6 +103,17 @@ static int br_device_event(struct notifier_block
*unused, unsigned long event, v
case NETDEV_PRE_TYPE_CHANGE:
/* Forbid underlaying device to change its type. */
return NOTIFY_BAD;
+
+ case NETDEV_NOTIFY_PEERS:
+ /* Send gratuitous arp while bond,a port of bridge, changes
*/
+ if (dev->priv_flags & IFF_BONDING && dev->flags &
IFF_MASTER) {
+ in_dev = __in_dev_get_rtnl(br->dev);
+
+ ASSERT_RTNL();
+ if (in_dev)
+ inetdev_send_gratuitous_arp(br->dev,
in_dev);
+ }
+ break;
}
/* Events that may cause spanning tree to refresh */
--
1.7.1
[-- Attachment #1.2: Type: text/html, Size: 2151 bytes --]
[-- Attachment #2: 0001-bridge-should-send-gratuitous-ARP-to-notify-peer-whi.patch --]
[-- Type: application/octet-stream, Size: 1678 bytes --]
From 3040990542ca5e4276f9d9215d4556c9e22cc581 Mon Sep 17 00:00:00 2001
From: Xing.Qingjie <xqjcool@gmail.com>
Date: Wed, 6 Sep 2017 04:03:25 +0800
Subject: [PATCH] bridge should send gratuitous ARP to notify peer while a bond, which
is a port of this bridge, changes.
We create bond0 and add eth0ð1 as slaves. Eth0 is active. Then we add bond0
into br0 as a bridge port. Br0 has ip address. When eth0 is down, after bond0's
failover eth1 become active. At this moment, we need br0 send a gratuitous ARP
to notify peer to update ARP table.
Signed-off-by: Xing.Qingjie <xqjcool@gmail.com>
---
net/bridge/br_notify.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index a76b621..90cf123 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c
@@ -34,6 +34,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
struct net_device *dev = ptr;
struct net_bridge_port *p;
struct net_bridge *br;
+ struct in_device *in_dev;
bool changed_addr;
int err;
@@ -102,6 +103,17 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
case NETDEV_PRE_TYPE_CHANGE:
/* Forbid underlaying device to change its type. */
return NOTIFY_BAD;
+
+ case NETDEV_NOTIFY_PEERS:
+ /* Send gratuitous arp while bond,a port of bridge, changes */
+ if (dev->priv_flags & IFF_BONDING && dev->flags & IFF_MASTER) {
+ in_dev = __in_dev_get_rtnl(br->dev);
+
+ ASSERT_RTNL();
+ if (in_dev)
+ inetdev_send_gratuitous_arp(br->dev, in_dev);
+ }
+ break;
}
/* Events that may cause spanning tree to refresh */
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:40 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, alan, peterz, netdev, Laurent Pinchart,
tglx, Mauro Carvalho Chehab, torvalds, Elena Reshetova,
linux-media
In-Reply-To: <20180106090907.GG4380@kroah.com>
On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
> > Static analysis reports that 'index' may be a user controlled value that
> > is used as a data dependency to read 'pin' from the
> > 'selector->baSourceID' array. In order to avoid potential leaks of
> > kernel memory values, block speculative execution of the instruction
> > stream that could issue reads based on an invalid value of 'pin'.
> >
> > Based on an original patch by Elena Reshetova.
> >
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 3e7e283a44a8..7442626dc20e 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -22,6 +22,7 @@
> > #include <linux/mm.h>
> > #include <linux/wait.h>
> > #include <linux/atomic.h>
> > +#include <linux/compiler.h>
> >
> > #include <media/v4l2-common.h>
> > #include <media/v4l2-ctrls.h>
> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> > struct uvc_entity *iterm = NULL;
> > u32 index = input->index;
> > int pin = 0;
> > + __u8 *elem;
> >
> > if (selector == NULL ||
> > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
> > break;
> > }
> > pin = iterm->id;
> > - } else if (index < selector->bNrInPins) {
> > - pin = selector->baSourceID[index];
> > + } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
> > + selector->bNrInPins))) {
> > + pin = *elem;
>
> I dug through this before, and I couldn't find where index came from
> userspace, I think seeing the coverity rule would be nice.
Ok, I take it back, this looks correct. Ugh, the v4l ioctl api is
crazy complex (rightfully so), it's amazing that coverity could navigate
that whole thing :)
While I'm all for fixing this type of thing, I feel like we need to do
something "else" for this as playing whack-a-mole for this pattern is
going to be a never-ending battle for all drivers for forever. Either
we need some way to mark this data path to make it easy for tools like
sparse to flag easily, or we need to catch the issue in the driver
subsystems, which unfortunatly, would harm the drivers that don't have
this type of issue (like here.)
I'm guessing that other operating systems, which don't have the luxury
of auditing all of their drivers are going for the "big hammer in the
subsystem" type of fix, right?
I don't have a good answer for this, but if there was some better way to
rewrite these types of patterns to just prevent the need for the
nospec_array_ptr() type thing, that might be the best overall for
everyone. Much like ebpf did with their changes. That way a simple
coccinelle rule would be able to catch the pattern and rewrite it.
Or am I just dreaming?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 10/18] qla2xxx: prevent bounds-check bypass via speculative execution
From: Greg KH @ 2018-01-06 9:42 UTC (permalink / raw)
To: Dan Williams
Cc: linux-kernel, linux-arch, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, peterz, netdev, qla2xxx-upstream,
tglx, torvalds, Elena Reshetova, alan
In-Reply-To: <20180106090322.GF4380@kroah.com>
On Sat, Jan 06, 2018 at 10:03:22AM +0100, Greg KH wrote:
> On Fri, Jan 05, 2018 at 05:10:48PM -0800, Dan Williams wrote:
> > Static analysis reports that 'handle' may be a user controlled value
> > that is used as a data dependency to read 'sp' from the
> > 'req->outstanding_cmds' array. In order to avoid potential leaks of
> > kernel memory values, block speculative execution of the instruction
> > stream that could issue reads based on an invalid value of 'sp'. In this
> > case 'sp' is directly dereferenced later in the function.
>
> I'm pretty sure that 'handle' comes from the hardware, not from
> userspace, from what I can tell here. If we want to start auditing
> __iomem data sources, great! But that's a bigger task, and one I don't
> think we are ready to tackle...
And as Peter Zijlstra has already mentioned, if we have to look at those
codepaths, USB drivers are the first up for that mess, so having access
to the coverity rules would be a great help in starting that effort.
thanks,
greg k-h
^ permalink raw reply
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: Jiri Pirko @ 2018-01-06 9:48 UTC (permalink / raw)
To: David Ahern
Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <20180106080728.GA2099@nanopsycho>
Sat, Jan 06, 2018 at 09:07:28AM CET, jiri@resnulli.us wrote:
>Sat, Jan 06, 2018 at 04:57:21AM CET, dsahern@gmail.com wrote:
>>On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Currently the filters added to qdiscs are independent. So for example if you
>>> have 2 netdevices and you create ingress qdisc on both and you want to add
>>> identical filter rules both, you need to add them twice. This patchset
>>> makes this easier and mainly saves resources allowing to share all filters
>>> within a qdisc - I call it a "filter block". Also this helps to save
>>> resources when we do offload to hw for example to expensive TCAM.
>>>
>>> So back to the example. First, we create 2 qdiscs. Both will share
>>> block number 22. "22" is just an identification. If we don't pass any
>>> block number, a new one will be generated by kernel:
>>>
>>> $ tc qdisc add dev ens7 ingress block 22
>>> ^^^^^^^^
>>> $ tc qdisc add dev ens8 ingress block 22
>>> ^^^^^^^^
>>>
>>> Now if we list the qdiscs, we will see the block index in the output:
>>>
>>> $ tc qdisc
>>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
>>> qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
>>>
>>>
>>> To make is more visual, the situation looks like this:
>>>
>>> ens7 ingress qdisc ens7 ingress qdisc
>>> | |
>>> | |
>>> +----------> block 22 <----------+
>>>
>>> Unlimited number of qdiscs may share the same block.
>>>
>>> Now we can add filter using the block index:
>>>
>>> $ tc filter add block 22 protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>>
>>>
>>> Note we cannot use the qdisc for filter manipulations for shared blocks:
>>>
>>> $ tc filter add dev ens8 ingress protocol ip pref 1 flower dst_ip 192.168.100.2 action drop
>>> Error: Cannot work with shared block, please use block index.
>>>
>>>
>>> We will see the same output if we list filters for ingress qdisc of
>>> ens7 and ens8, also for the block 22:
>>>
>>> $ tc filter show block 22
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens7 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens8 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>
>>I like the API and output shown here, but I am not getting that with the
>>patches.
>>
>>In this example, I am using 42 for the block id:
>>
>>$ tc qdisc show dev eth2
>>qdisc mq 0: root
>>qdisc pfifo_fast 0: parent :2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>>1 1 1
>>qdisc pfifo_fast 0: parent :1 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>>1 1 1
>>qdisc ingress ffff: parent ffff:fff1 block 42
>>
>>It allows me to add a filter using the device:
>>$ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
>>192.168.101.2 action drop
>>$ echo $?
>>0
>
>Yes, because the block is not shared yet. You have it only for one
>qdisc. As long as you have that, the "filter add dev" api still works.
>It stops working when you add another qdisc to that block.
Or, do you think it should work like:
$ tc qdisc add dev ens8 ingress
$ tc qdisc
qdisc ingress ffff: dev ens8 parent ffff:fff1
$ tc qdisc add dev ens7 ingress block 22
$ tc qdisc
qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
qdisc ingress ffff: dev ens8 parent ffff:fff1
And the only shareable block is the one which I spefify block index for?
And for that, I have to always use block index handle for filter
add/del/get?
^ permalink raw reply
* Re: [PATCH 08/18] carl9170: prevent bounds-check bypass via speculative execution
From: Sergei Shtylyov @ 2018-01-06 10:01 UTC (permalink / raw)
To: Dan Williams, linux-kernel
Cc: linux-arch, peterz, netdev, linux-wireless, Elena Reshetova,
gregkh, Christian Lamparter, tglx, torvalds, Kalle Valo, alan
In-Reply-To: <151520103755.32271.6819511294540882298.stgit@dwillia2-desk3.amr.corp.intel.com>
Hello!
On 1/6/2018 4:10 AM, Dan Williams wrote:
> Static analysis reports that 'queue' may be a user controlled value that
> is used as a data dependency to read from the 'ar9170_qmap' array. In
> order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue reads
> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> 'ar->edcf' array.
>
> Based on an original patch by Elena Reshetova.
>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/net/wireless/ath/carl9170/main.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..0ff34cbe2b62 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -41,6 +41,7 @@
> #include <linux/module.h>
> #include <linux/etherdevice.h>
> #include <linux/random.h>
> +#include <linux/compiler.h>
> #include <net/mac80211.h>
> #include <net/cfg80211.h>
> #include "hw.h"
> @@ -1384,11 +1385,12 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> const struct ieee80211_tx_queue_params *param)
> {
> struct ar9170 *ar = hw->priv;
> + const u8 *elem;
> int ret;
>
> mutex_lock(&ar->mutex);
> - if (queue < ar->hw->queues) {
> - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> + if ((elem = nospec_array_ptr(ar9170_qmap, queue, ar->hw->queues))) {
I bet this causes checkpatch.pl to complain. I don't see a dire need to
upset it here, the assignment may well precede *if*...
> + memcpy(&ar->edcf[*elem], param, sizeof(*param));
> ret = carl9170_set_qos(ar);
> } else {
> ret = -EINVAL;
>
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 09/18] p54: prevent bounds-check bypass via speculative execution
From: Sergei Shtylyov @ 2018-01-06 10:01 UTC (permalink / raw)
To: Dan Williams, linux-kernel
Cc: linux-arch, peterz, netdev, linux-wireless, Elena Reshetova,
gregkh, Christian Lamparter, tglx, torvalds, Kalle Valo, alan
In-Reply-To: <151520104323.32271.6614158873750932410.stgit@dwillia2-desk3.amr.corp.intel.com>
On 1/6/2018 4:10 AM, Dan Williams wrote:
> Static analysis reports that 'queue' may be a user controlled value that
> is used as a data dependency to read from the 'priv->qos_params' array.
> In order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue reads
> based on an invalid result of 'priv->qos_params[queue]'.
>
> Based on an original patch by Elena Reshetova.
>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/net/wireless/intersil/p54/main.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/p54/main.c b/drivers/net/wireless/intersil/p54/main.c
> index ab6d39e12069..85c9cbee35fc 100644
> --- a/drivers/net/wireless/intersil/p54/main.c
> +++ b/drivers/net/wireless/intersil/p54/main.c
[...]
> @@ -411,12 +412,13 @@ static int p54_conf_tx(struct ieee80211_hw *dev,
> const struct ieee80211_tx_queue_params *params)
> {
> struct p54_common *priv = dev->priv;
> + struct p54_edcf_queue_param *p54_q;
> int ret;
>
> mutex_lock(&priv->conf_mutex);
> - if (queue < dev->queues) {
> - P54_SET_QUEUE(priv->qos_params[queue], params->aifs,
> - params->cw_min, params->cw_max, params->txop);
> + if ((p54_q = nospec_array_ptr(priv->qos_params, queue, dev->queues))) {
Same complaint here...
> + P54_SET_QUEUE(p54_q[0], params->aifs, params->cw_min,
> + params->cw_max, params->txop);
> ret = p54_set_edcf(priv);
> } else
> ret = -EINVAL;
>
MBR, Sergei
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox