* [PATCH net] macsec: avoid heap overflow in skb_to_sgvec on receive
From: Sabrina Dubroca @ 2017-04-25 15:19 UTC (permalink / raw)
To: netdev; +Cc: Sabrina Dubroca, Jason A. Donenfeld
The previous fix for this issue, commit 4d6fa57b4dab ("macsec: avoid
heap overflow in skb_to_sgvec"), doesn't really fix much. It removed the
NETIF_F_FRAGLIST flag from MACsec device features, but this flag isn't
checked anywhere in the codepaths leading to a macsec_decrypt() call.
On TX, macsec could already handle a frag_list because an skb with a
frag_list will get linearized in skb_copy_expand() since it lacks the
necessary tailroom. Removing the NETIF_F_FRAGLIST makes sure
macsec_encrypt() will never see a frag_list.
On RX, we can simply get the number of necessary scatterlist items by
calling skb_cow_data().
Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Fixes: CVE-2017-7477
Reported-by: "Jason A. Donenfeld" <Jason@zx2c4.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
drivers/net/macsec.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..c9cc40d2349c 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request *base, int err)
static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
unsigned char **iv,
- struct scatterlist **sg)
+ struct scatterlist **sg,
+ int nfrags)
{
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
- size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+ size += sizeof(struct scatterlist) * nfrags;
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -723,7 +724,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
- req = macsec_alloc_req(tx_sa->key.tfm, &iv, &sg);
+ req = macsec_alloc_req(tx_sa->key.tfm, &iv, &sg, MAX_SKB_FRAGS + 1);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -921,13 +922,21 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
struct aead_request *req;
struct macsec_eth_header *hdr;
u16 icv_len = secy->icv_len;
+ struct sk_buff *trailer;
+ int nfrags;
macsec_skb_cb(skb)->valid = false;
skb = skb_share_check(skb, GFP_ATOMIC);
if (!skb)
return ERR_PTR(-ENOMEM);
- req = macsec_alloc_req(rx_sa->key.tfm, &iv, &sg);
+ nfrags = skb_cow_data(skb, 0, &trailer);
+ if (nfrags < 0) {
+ kfree_skb(skb);
+ return ERR_PTR(nfrags);
+ }
+
+ req = macsec_alloc_req(rx_sa->key.tfm, &iv, &sg, nfrags);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -936,7 +945,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
hdr = (struct macsec_eth_header *)skb->data;
macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
- sg_init_table(sg, MAX_SKB_FRAGS + 1);
+ sg_init_table(sg, nfrags);
skb_to_sgvec(skb, sg, 0, skb->len);
if (hdr->tci_an & MACSEC_TCI_E) {
--
2.12.2
^ permalink raw reply related
* Re: pull-request: can 2017-04-25
From: David Miller @ 2017-04-25 15:21 UTC (permalink / raw)
To: mkl; +Cc: netdev, linux-can, kernel
In-Reply-To: <20170425121645.9946-1-mkl@pengutronix.de>
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Tue, 25 Apr 2017 14:16:42 +0200
> this is a pull request of three patches for net/master.
>
> There are two patches by Stephane Grosjean for that add a new variant to the
> PCAN-Chip USB driver. The other patch is by Maksim Salau, which swtiches the
> memory for USB transfers from heap to stack.
I think you meant "from stack to heap", but anyways pulled thanks!
^ permalink raw reply
* [PATCH] macsec: dynamically allocate space for sglist
From: Jason A. Donenfeld @ 2017-04-25 15:23 UTC (permalink / raw)
To: Netdev, LKML, David Miller, stable, security
Cc: Jason A. Donenfeld, Sabrina Dubroca
In-Reply-To: <CAHmME9rZ29X=R-quViXaEO_ouxeB2EpzR2bkR93rrN4n--QDqw@mail.gmail.com>
We call skb_cow_data, which is good anyway to ensure we can actually
modify the skb as such (another error from prior). Now that we have the
number of fragments required, we can safely allocate exactly that amount
of memory.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: security@kernel.org
Cc: stable@vger.kernel.org
---
drivers/net/macsec.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..56dafdee4c9c 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -617,7 +617,8 @@ static void macsec_encrypt_done(struct crypto_async_request *base, int err)
static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
unsigned char **iv,
- struct scatterlist **sg)
+ struct scatterlist **sg,
+ int num_frags)
{
size_t size, iv_offset, sg_offset;
struct aead_request *req;
@@ -629,7 +630,7 @@ static struct aead_request *macsec_alloc_req(struct crypto_aead *tfm,
size = ALIGN(size, __alignof__(struct scatterlist));
sg_offset = size;
- size += sizeof(struct scatterlist) * (MAX_SKB_FRAGS + 1);
+ size += sizeof(struct scatterlist) * num_frags;
tmp = kmalloc(size, GFP_ATOMIC);
if (!tmp)
@@ -649,6 +650,7 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
{
int ret;
struct scatterlist *sg;
+ struct sk_buff *trailer;
unsigned char *iv;
struct ethhdr *eth;
struct macsec_eth_header *hh;
@@ -723,7 +725,14 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
return ERR_PTR(-EINVAL);
}
- req = macsec_alloc_req(tx_sa->key.tfm, &iv, &sg);
+ ret = skb_cow_data(skb, 0, &trailer);
+ if (unlikely(ret < 0)) {
+ macsec_txsa_put(tx_sa);
+ kfree_skb(skb);
+ return ERR_PTR(ret);
+ }
+
+ req = macsec_alloc_req(tx_sa->key.tfm, &iv, &sg, ret);
if (!req) {
macsec_txsa_put(tx_sa);
kfree_skb(skb);
@@ -917,6 +926,7 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
{
int ret;
struct scatterlist *sg;
+ struct sk_buff *trailer;
unsigned char *iv;
struct aead_request *req;
struct macsec_eth_header *hdr;
@@ -927,7 +937,12 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
if (!skb)
return ERR_PTR(-ENOMEM);
- req = macsec_alloc_req(rx_sa->key.tfm, &iv, &sg);
+ ret = skb_cow_data(skb, 0, &trailer);
+ if (unlikely(ret < 0)) {
+ kfree_skb(skb);
+ return ERR_PTR(ret);
+ }
+ req = macsec_alloc_req(rx_sa->key.tfm, &iv, &sg, ret);
if (!req) {
kfree_skb(skb);
return ERR_PTR(-ENOMEM);
@@ -2716,7 +2731,7 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
}
#define MACSEC_FEATURES \
- (NETIF_F_SG | NETIF_F_HIGHDMA)
+ (NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST)
static struct lock_class_key macsec_netdev_addr_lock_key;
static int macsec_dev_init(struct net_device *dev)
--
2.12.2
^ permalink raw reply related
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Sergei Shtylyov @ 2017-04-25 15:23 UTC (permalink / raw)
To: Jon Mason
Cc: Mark Rutland, Andrew Lunn, Florian Fainelli, Scott Branden,
Vivien Didelot, Jon Mason, Network Development, open list,
Eric Anholt,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring, BCM Kernel Feedback, Ray Jui, linux-arm-kernel
In-Reply-To: <CAC3K-4qn5oCXvyMCkFESGqMHqHXGiNUW6wfRx2LUZ8qZ39qroQ@mail.gmail.com>
Hello!
On 04/25/2017 06:15 PM, Jon Mason wrote:
>>> Cygnus has a single amac controller connected to the B53 switch with 2
>>> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
>>> the external ethernet jacks.
[...]
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> ---
>>> arch/arm/boot/dts/bcm-cygnus.dtsi | 60
>>> ++++++++++++++++++++++++++++++++++
>>> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
>>> 2 files changed, 68 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> index 009f1346b817..318899df9972 100644
>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
[...]
>>> @@ -295,6 +345,16 @@
>>> status = "disabled";
>>> };
>>>
>>> + eth0: enet@18042000 {
>>> + compatible = "brcm,amac";
>>> + reg = <0x18042000 0x1000>,
>>> + <0x18110000 0x1000>;
>>> + reg-names = "amac_base", "idm_base";
>>
>>
>> I don't think "_base" suffixes are necessary here.
>
> 100% necessary, per the driver. See
> drivers/net/ethernet/broadcom/bgmac-platform.c
I'd recommend to fix the driver/bindings then...
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt
From: Alexander Kochetkov @ 2017-04-25 15:25 UTC (permalink / raw)
To: David Miller
Cc: Florian Fainelli, netdev, LKML, Sergei Shtylyov, Roger Quadros,
Madalin Bucur, Alexandre Belloni
In-Reply-To: <20170425.103644.75674234677063517.davem@davemloft.net>
Hello David!
> 25 апр. 2017 г., в 17:36, David Miller <davem@davemloft.net> написал(а):
>
> So... what are we doing here?
>
> My understanding is that this should fix the same problem that commit
> 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: micrel: Disable auto
> negotiation on startup") fixed and that this micrel commit should thus
> be reverted to improve MAC startup times which regressed.
>
> Florian, any guidance?
Yes, this should be done.
I aksed Alexandre to check if 99f81afc139c6edd14d77a91ee91685a414a1c66 ("phy: micrel: Disable auto
negotiation on startup») can be reverted, and he answered what it may do that
sometime this/next week.
Alexander.
^ permalink raw reply
* Re: [ISSUE: sky2 - rx error] Link stops working under heavy traffic load connected to a mv88e6176
From: Stephen Hemminger @ 2017-04-25 15:27 UTC (permalink / raw)
To: Rafa Corvillo; +Cc: netdev
In-Reply-To: <58F9FD64.80506@aoifes.com>
On Fri, 21 Apr 2017 14:39:00 +0200
Rafa Corvillo <rafael.corvillo@aoifes.com> wrote:
> We are working in an ARMv7 embedded system running kernel 4.9 (LEDE build).
> It is an imx6 board with 2 ethernet interfaces. One of them is connected to
> a Marvell switch.
>
> The schema of the system is the following:
>
> +-------------------+ eth0
> | +--+
> | | |
> | Embedded system +--+
> | |
> | ARMv7 |
> | | Marvell 88E8057(sky2) +-------------+
> | +--+ +--+ +--+ eth1
> | | +---------------------+ | | +------+
> | +--+ CPU port +--+ mv88e6176 +--+
> +------+--+---------+ | |
> emulated| | | |
> GPIO +--+ +--+ +--+ eth2
> MDIO +-----------------------------------+ | | +------+
> MDIO +--+ +--+
> +-------------+
>
> There is a bridge (br-lan) which includes eth0/eth1/eth2
>
> If I connect the eth1/eth2, the link is up and I can do ping through it.
> But, once
> I start sending a heavy traffic load the link fails and the kernel sends the
> following messages:
>
> [ 48.557140] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.564964] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.572110] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.579263] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.586417] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.593573] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 48.600718] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 54.877567] net_ratelimit: 6 callbacks suppressed
> [ 54.882293] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
> [ 61.413552] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
> length 1518
The status error bits are in sky2.h
0x5f20010 is
05f2 frame length => 1522
0010 Too long err
That means the packet was longer than the configured MTU.
You are probably getting packets with VLAN tag but have not configured
a VLAN.
^ permalink raw reply
* Re: [PATCH net-next 1/1] net sched actions: Complete the JUMPX opcode
From: David Miller @ 2017-04-25 15:30 UTC (permalink / raw)
To: jhs; +Cc: netdev, jiri, xiyou.wangcong, daniel, alexei.starovoitov
In-Reply-To: <1492967848-10165-1-git-send-email-jhs@emojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sun, 23 Apr 2017 13:17:28 -0400
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> per discussion at netconf/netdev:
> When we have an action that is capable of branching (example a policer),
> we can achieve a continuation of the action graph by programming a
> "continue" where we find an exact replica of the same filter rule with a lower
> priority and the remainder of the action graph. When you have 100s of thousands
> of filters which require such a feature it gets very inefficient to do two
> lookups.
>
> This patch completes a leftover feature of action codes. Its time has come.
>
> Example below where a user labels packets with a different skbmark on ingress
> of a port depending on whether they have/not exceeded the configured rate.
> This mark is then used to make further decisions on some egress port.
>
> #rate control, very low so we can easily see the effect
> sudo $TC actions add action police rate 1kbit burst 90k \
> conform-exceed pipe/jump 2 index 10
> # skbedit index 11 will be used if the user conforms
> sudo $TC actions add action skbedit mark 11 ok index 11
> # skbedit index 12 will be used if the user does not conform
> sudo $TC actions add action skbedit mark 12 ok index 12
>
> #lets bind the user ..
> sudo $TC filter add dev $ETH parent ffff: protocol ip prio 8 u32 \
> match ip dst 127.0.0.8/32 flowid 1:10 \
> action police index 10 \
> action skbedit index 11 \
> action skbedit index 12
>
> #run a ping -f and see what happens..
> #
> jhs@foobar:~$ sudo $TC -s filter ls dev $ETH parent ffff: protocol ip
> filter pref 8 u32
> filter pref 8 u32 fh 800: ht divisor 1
> filter pref 8 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10 (rule hit 2800 success 1005)
...
> Not bad, about 28% non-conforming packets..
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Applied, thanks Jamal.
^ permalink raw reply
* Re: pull-request: can 2017-04-25
From: Marc Kleine-Budde @ 2017-04-25 15:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-can, kernel
In-Reply-To: <20170425.112126.232541678435322858.davem@davemloft.net>
[-- Attachment #1.1: Type: text/plain, Size: 811 bytes --]
On 04/25/2017 05:21 PM, David Miller wrote:
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Date: Tue, 25 Apr 2017 14:16:42 +0200
>
>> this is a pull request of three patches for net/master.
>>
>> There are two patches by Stephane Grosjean for that add a new variant to the
>> PCAN-Chip USB driver. The other patch is by Maksim Salau, which swtiches the
>> memory for USB transfers from heap to stack.
>
> I think you meant "from stack to heap", but anyways pulled thanks!
Yes and that's what the patch does.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net] team: fix memory leaks
From: David Miller @ 2017-04-25 15:35 UTC (permalink / raw)
To: bianpan2016; +Cc: jiri, netdev, linux-kernel
In-Reply-To: <1493029756-13171-1-git-send-email-bianpan2016@163.com>
From: Pan Bian <bianpan2016@163.com>
Date: Mon, 24 Apr 2017 18:29:16 +0800
> In functions team_nl_send_port_list_get() and
> team_nl_send_options_get(), pointer skb keeps the return value of
> nlmsg_new(). When the call to genlmsg_put() fails, the memory is not
> freed(). This will result in memory leak bugs.
>
> Fixes: 9b00cf2d1024 ("team: implement multipart netlink messages for options transfers")
> Signed-off-by: Pan Bian <bianpan2016@163.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH RFC] ptr_ring: add ptr_ring_unconsume
From: Michael S. Tsirkin @ 2017-04-25 15:35 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev
In-Reply-To: <91ae69e6-4dac-3db2-4778-c4163dfe6f91@redhat.com>
On Tue, Apr 25, 2017 at 12:07:01PM +0800, Jason Wang wrote:
>
>
> On 2017年04月24日 20:00, Michael S. Tsirkin wrote:
> > On Mon, Apr 24, 2017 at 07:54:18PM +0800, Jason Wang wrote:
> > > On 2017年04月24日 07:28, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 18, 2017 at 11:07:42AM +0800, Jason Wang wrote:
> > > > > On 2017年04月17日 07:19, Michael S. Tsirkin wrote:
> > > > > > Applications that consume a batch of entries in one go
> > > > > > can benefit from ability to return some of them back
> > > > > > into the ring.
> > > > > >
> > > > > > Add an API for that - assuming there's space. If there's no space
> > > > > > naturally we can't do this and have to drop entries, but this implies
> > > > > > ring is full so we'd likely drop some anyway.
> > > > > >
> > > > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Jason, in my mind the biggest issue with your batching patchset is the
> > > > > > backet drops on disconnect. This API will help avoid that in the common
> > > > > > case.
> > > > > Ok, I will rebase the series on top of this. (Though I don't think we care
> > > > > the packet loss).
> > > > E.g. I care - I often start sending packets to VM before it's
> > > > fully booted. Several vhost resets might follow.
> > > Ok.
> > >
> > > > > > I would still prefer that we understand what's going on,
> > > > > I try to reply in another thread, does it make sense?
> > > > >
> > > > > > and I would
> > > > > > like to know what's the smallest batch size that's still helpful,
> > > > > Yes, I've replied in another thread, the result is:
> > > > >
> > > > >
> > > > > no batching 1.88Mpps
> > > > > RX_BATCH=1 1.93Mpps
> > > > > RX_BATCH=4 2.11Mpps
> > > > > RX_BATCH=16 2.14Mpps
> > > > > RX_BATCH=64 2.25Mpps
> > > > > RX_BATCH=256 2.18Mpps
> > > > Essentially 4 is enough, other stuf looks more like noise
> > > > to me. What about 2?
> > > The numbers are pretty stable, so probably not noise. Retested on top of
> > > batch zeroing:
> > >
> > > no 1.97Mpps
> > > 1 2.09Mpps
> > > 2 2.11Mpps
> > > 4 2.16Mpps
> > > 8 2.19Mpps
> > > 16 2.21Mpps
> > > 32 2.25Mpps
> > > 64 2.30Mpps
> > > 128 2.21Mpps
> > > 256 2.21Mpps
> > >
> > > 64 performs best.
> > >
> > > Thanks
> > OK but it might be e.g. a function of the ring size, host cache size or
> > whatever. As we don't really understand the why, if we just optimize for
> > your setup we risk regressions in others. 64 entries is a lot, it
> > increases the queue size noticeably. Could this be part of the effect?
> > Could you try changing the queue size to see what happens?
>
> I increase tx_queue_len to 1100, but only see less than 1% improvement on
> pps number (batch = 1) in my machine. If you care about the regression, we
> probably can leave the choice to user through e.g module parameter. But I'm
> afraid we have already had too much choices for them. Or I can test this
> with different CPU types.
>
> Thanks
>
I agree here. Let's keep it a constant. Testing on more machines would
be nice but not strictly required. I just dislike not understanding why
it helps because it means we can easily break it by mistake. So my only
request really is that you wrap access to this internal buffer in an
API. Let's see - I think we need
struct vhost_net_buf
vhost_net_buf_get_ptr
vhost_net_buf_get_size
vhost_net_buf_is_empty
vhost_net_buf_peek
vhost_net_buf_consume
vhost_net_buf_produce
--
MST
^ permalink raw reply
* Re: [PATCH net] macsec: avoid heap overflow in skb_to_sgvec on receive
From: Jason A. Donenfeld @ 2017-04-25 15:39 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: Netdev
In-Reply-To: <f50ef5b9e525d83938f5e419557abb0307ee023b.1493133423.git.sd@queasysnail.net>
Hi Sabrina,
I think I may have beaten you to the punch here by a few minutes. :)
The difference between our two versions is that you don't re-add the
FRAGLIST attribute, whereas my patch does, and then it does the
dynamic allocation. I suspect this might be a bit more robust. It also
ensures that skb_cow_data is called on both paths. So perhaps let's
roll with mine?
Jason
^ permalink raw reply
* Aw: Re: [Intel-wired-lan] [PATCH] ixgbe: initialize u64_stats_sync structures early at ixgbe_probe
From: Lino Sanfilippo @ 2017-04-25 15:39 UTC (permalink / raw)
To: Alexander Duyck
Cc: Singh, Krishneil K, Song, Liwei (Wind River), Kirsher, Jeffrey T,
netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAKgT0UeTdY9j1qDHP=DscgAZF2pPFCb+=ACySqKBK+UuBsaNBA@mail.gmail.com>
Hi,
> This patch doesn't look right to me. I would suggest rejecting it.
>
> The call to initialize the stats should be done when the ring is
> allocated, not in ixgbe_probe(). This should probably be done in
> ixgbe_alloc_q_vector() instead.
>
AFAICS ixgbe_alloc_q_vector() is also called in probe() (by ixgbe_init_interrupt_scheme()).
Furthermore it is also called in resume() which would lead to multiple initialization of
the u64_stats_sync in case of resume.
IMHO the u64_stats_sync variables have to be initialized before register_netdev() is called
since this is the point from which userspace can call ixgbe_get_stats64(). I would say the
best place to do so is the probe() function as it is done in this patch.
Just my 2 cents.
Regards,
Lino
^ permalink raw reply
* Re: [PATCH] IB/IPoIB: Check the headroom size
From: Doug Ledford @ 2017-04-25 15:40 UTC (permalink / raw)
To: Or Gerlitz, Erez Shitrit, Paolo Abeni
Cc: Honggang LI, Erez Shitrit,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Netdev List, David Miller
In-Reply-To: <CAJ3xEMgwS1Bq8+eZC1iAr6xi8ZPHrchsOJ5r4LNJxR8P+6VipA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 2017-04-25 at 17:39 +0300, Or Gerlitz wrote:
> On Tue, Apr 25, 2017 at 2:43 PM, Erez Shitrit <erezsh-LDSdmyG8hGUWn1LaSxwUnA@public.gmane.org
> .il> wrote:
> >
> > On Tue, Apr 25, 2017 at 2:14 PM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > wrote:
>
> >
> > >
> > > thanks for the info. Is this bug there since ipoib/bonding day
> > > one (and hence my bug...)
> > > or was indeed introduced later? if later, can you explain how
> > > fc791b633515 introduced that or you only know it by bisection?
>
> >
> > commit "fc791b633515" changes the size of the dev_hardlen to be 24
> > and
> > required 24 extra bytes in the skb, before it was only 4, if skb is
> > aligned to eth "mode" it already has 14 bytes for hard-header.
> > So only after that commit we have the issue.
>
> If got you right, Paolo's commit introduced a regression, so we (I
> guess you and
> Paolo) need to either solve it or we (community) should consider a
> revert, please suggest.
It's a little more complex than that. Paolo's commit *re-introduced* a
regression. If you recall, long ago the IPoIB layer stuck the dgid
into the skb, then pulled it out later. However, we didn't actually
declare things properly back then, but it worked anyway. Then we had
the commit you authored to start using the skb->cb to store the dgid,
and our usage of hard header dropped to only 4 bytes. Paolo's commit
went back to the old way of doing things, but also did the proper
accounting and setup to tell the netstack what we were doing (which the
initial implementation never did IIRC). So, this issue should be
reproducible either after Paolo's commit or with any kernel prior to
your commit to use the skb->cb area to store the DGID, but it probably
requires the specific series of actions in this bug to trigger it. A
normal, clean shutdown of the interface doesn't demonstrate the issue.
> The bug is now in stable and distro kernels, so please act.
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next 0/2] l2tp: add informations about l2tpeth interfaces in /sys
From: David Miller @ 2017-04-25 15:42 UTC (permalink / raw)
To: g.nault; +Cc: netdev, jchapman
In-Reply-To: <cover.1493035407.git.g.nault@alphalink.fr>
From: Guillaume Nault <g.nault@alphalink.fr>
Date: Mon, 24 Apr 2017 14:16:04 +0200
> Patch #1 lets userspace retrieve the naming scheme of an l2tpeth
> interface, using /sys/class/net/<iface>/name_assign_type.
>
> Patch #2 adds the DEVTYPE field in /sys/class/net/<iface>/uevent so
> that userspace can reliably know if a device is an l2tpeth interface.
Looks great, series applied, thanks!
^ permalink raw reply
* Re: [PATCH 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
From: Sergei Shtylyov @ 2017-04-25 15:42 UTC (permalink / raw)
To: Jason A. Donenfeld, netdev, linux-kernel, davem, David.Laight,
kernel-hardening
In-Reply-To: <20170425140809.23881-1-Jason@zx2c4.com>
Hello!
On 04/25/2017 05:08 PM, Jason A. Donenfeld wrote:
> This is a defense-in-depth measure in response to bugs like
> 4d6fa57b4dab0d77f4d8e9d9c73d1e63f6fe8fee.
You need to also specify the summary line enclosed in (""). And it's
enough to specify 12 digits of SHA1 ID...
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: Add the ethernet and ethernet PHY to the cygnus core DT.
From: Jon Mason @ 2017-04-25 15:43 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Eric Anholt, Florian Fainelli, Vivien Didelot, Andrew Lunn,
Network Development, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-arm-kernel, open list, BCM Kernel Feedback, Ray Jui,
Scott Branden, Jon Mason
In-Reply-To: <44d0c48c-d80f-271e-0e19-ac87a53e2e53@cogentembedded.com>
On Tue, Apr 25, 2017 at 11:23 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello!
>
> On 04/25/2017 06:15 PM, Jon Mason wrote:
>
>>>> Cygnus has a single amac controller connected to the B53 switch with 2
>>>> PHYs. On the BCM911360_EP platform, those two PHYs are connected to
>>>> the external ethernet jacks.
>
>
> [...]
>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>> ---
>>>> arch/arm/boot/dts/bcm-cygnus.dtsi | 60
>>>> ++++++++++++++++++++++++++++++++++
>>>> arch/arm/boot/dts/bcm911360_entphn.dts | 8 +++++
>>>> 2 files changed, 68 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> b/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> index 009f1346b817..318899df9972 100644
>>>> --- a/arch/arm/boot/dts/bcm-cygnus.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
>
> [...]
>>>>
>>>> @@ -295,6 +345,16 @@
>>>> status = "disabled";
>>>> };
>>>>
>>>> + eth0: enet@18042000 {
>>>> + compatible = "brcm,amac";
>>>> + reg = <0x18042000 0x1000>,
>>>> + <0x18110000 0x1000>;
>>>> + reg-names = "amac_base", "idm_base";
>>>
>>>
>>>
>>> I don't think "_base" suffixes are necessary here.
>>
>>
>> 100% necessary, per the driver. See
>> drivers/net/ethernet/broadcom/bgmac-platform.c
>
>
> I'd recommend to fix the driver/bindings then...
They're already in use in other device trees. So, we'd need to
support backward compatibility on them, thus removing any real benefit
to changing them.
>
> MBR, Sergei
>
^ permalink raw reply
* Re: [PATCH net] ipv6: move stub initialization after ipv6 setup completion
From: David Miller @ 2017-04-25 15:43 UTC (permalink / raw)
To: pabeni; +Cc: netdev, xiyou.wangcong, hannes
In-Reply-To: <fd7c85f650661466a782a71485f227f0b84454a7.1493035843.git.pabeni@redhat.com>
From: Paolo Abeni <pabeni@redhat.com>
Date: Mon, 24 Apr 2017 14:18:28 +0200
> The ipv6 stub pointer is currently initialized before the ipv6
> routing subsystem: a 3rd party can access and use such stub
> before the routing data is ready.
> Moreover, such pointer is not cleared in case of initialization
> error, possibly leading to dangling pointers usage.
>
> This change addresses the above moving the stub initialization
> at the end of ipv6 init code.
>
> Fixes: 5f81bd2e5d80 ("ipv6: export a stub for IPv6 symbols used by vxlan")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v2] net/packet: initialize val in packet_getsockopt()
From: David Miller @ 2017-04-25 15:44 UTC (permalink / raw)
To: glider; +Cc: dvyukov, kcc, edumazet, kuznet, linux-kernel, netdev
In-Reply-To: <20170424125914.43270-1-glider@google.com>
From: Alexander Potapenko <glider@google.com>
Date: Mon, 24 Apr 2017 14:59:14 +0200
> In the case getsockopt() is called with PACKET_HDRLEN and optlen < 4
> |val| remains uninitialized and the syscall may behave differently
> depending on its value. This doesn't have security consequences (as the
> uninit bytes aren't copied back), but it's still cleaner to initialize
> |val| and ensure optlen is not less than sizeof(int).
>
> This bug has been detected with KMSAN.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> v2: - if len < sizeof(int), make it 0
No, you should signal an error if the len is too small.
Returning zero bytes to userspace silently makes the user think that
he got the data he asked for.
^ permalink raw reply
* Re: [PATCH net v1 1/2] tipc: fix socket flow control accounting error at tipc_send_stream
From: David Miller @ 2017-04-25 15:45 UTC (permalink / raw)
To: parthasarathy.bhuvaragan; +Cc: netdev, tipc-discussion
In-Reply-To: <1493038843-30621-1-git-send-email-parthasarathy.bhuvaragan@ericsson.com>
From: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
Date: Mon, 24 Apr 2017 15:00:42 +0200
> Until now in tipc_send_stream(), we return -1 when the socket
> encounters link congestion even if the socket had successfully
> sent partial data. This is incorrect as the application resends
> the same the partial data leading to data corruption at
> receiver's end.
>
> In this commit, we return the partially sent bytes as the return
> value at link congestion.
>
> Fixes: 10724cc7bb78 ("tipc: redesign connection-level flow control")
> Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
> Reviewed-by: Jon Maloy <jon.maloy@ericsson.com>
Both patches #1 and #2 applied, thanks.
^ permalink raw reply
* Re: [PATCH net] macsec: avoid heap overflow in skb_to_sgvec on receive
From: Sabrina Dubroca @ 2017-04-25 15:51 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: Netdev
In-Reply-To: <CAHmME9rxdTEe1-5ApgUhdkqiJztSgJb0QuhV2uSemobeXm_oNg@mail.gmail.com>
2017-04-25, 17:39:09 +0200, Jason A. Donenfeld wrote:
> Hi Sabrina,
>
> I think I may have beaten you to the punch here by a few minutes. :)
I said I was going to post a patch.
Mail headers seem to disagree with you ;)
> The difference between our two versions is that you don't re-add the
> FRAGLIST attribute, whereas my patch does, and then it does the
> dynamic allocation. I suspect this might be a bit more robust. It also
> ensures that skb_cow_data is called on both paths. So perhaps let's
> roll with mine?
I don't see the "more robust" argument.
Unless I missed something, encrypt was already handling fragments
correctly. An skb with ->frag_list should have no skb_tailroom, so it
will be linearized skb_copy_expand().
--
Sabrina
^ permalink raw reply
* Re: net: heap out-of-bounds in fib6_clean_node/rt6_fill_node/fib6_age/fib6_prune_clone
From: David Ahern @ 2017-04-25 15:51 UTC (permalink / raw)
To: Andrey Konovalov, Dmitry Vyukov
Cc: Eric Dumazet, Mahesh Bandewar, Eric Dumazet, David Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, netdev, LKML, Cong Wang, syzkaller
In-Reply-To: <CAAeHK+zx6W_yxnoEQ2Pc1AT4uLXqYZaoi5oQBeuCntdGS38TQg@mail.gmail.com>
On 4/18/17 2:43 PM, Andrey Konovalov wrote:
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN
> Modules linked in:
> CPU: 1 PID: 4035 Comm: a.out Not tainted 4.11.0-rc7+ #250
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff880069809600 task.stack: ffff880062dc8000
> RIP: 0010:ip6_rt_cache_alloc+0xa6/0x560 net/ipv6/route.c:975
> RSP: 0018:ffff880062dced30 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: ffff8800670561c0 RCX: 0000000000000006
> RDX: 0000000000000003 RSI: ffff880062dcfb28 RDI: 0000000000000018
> RBP: ffff880062dced68 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff880062dcfb28 R14: dffffc0000000000 R15: 0000000000000000
> FS: 00007feebe37e7c0(0000) GS:ffff88006cb00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000205a0fe4 CR3: 000000006b5c9000 CR4: 00000000000006e0
> Call Trace:
> ip6_pol_route+0x1512/0x1f20 net/ipv6/route.c:1128
This one is fixed by:
commit 557c44be917c322860665be3d28376afa84aa936
Author: David Ahern <dsa@cumulusnetworks.com>
Date: Wed Apr 19 14:19:43 2017 -0700
net: ipv6: RTF_PCPU should not be settable from userspace
^ permalink raw reply
* [PATCH v5 1/5] skbuff: return -EMSGSIZE in skb_to_sgvec to prevent overflow
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
Cc: Jason A. Donenfeld
In-Reply-To: <20170425.111717.1973239615715123840.davem@davemloft.net>
This is a defense-in-depth measure in response to bugs like
4d6fa57b4dab ("macsec: avoid heap overflow in skb_to_sgvec")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
This is a resend of v4 with all the other child commits along with it.
net/core/skbuff.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f86bf69cfb8d..e75640006d78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3489,7 +3489,9 @@ void __init skb_init(void)
* @len: Length of buffer space to be mapped
*
* Fill the specified scatter-gather list with mappings/pointers into a
- * region of the buffer space attached to a socket buffer.
+ * region of the buffer space attached to a socket buffer. Returns either
+ * the number of scatterlist items used, or -EMSGSIZE if the contents
+ * could not fit.
*/
static int
__skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
@@ -3517,6 +3519,8 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]);
if ((copy = end - offset) > 0) {
skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
if (copy > len)
copy = len;
@@ -3537,6 +3541,9 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
end = start + frag_iter->len;
if ((copy = end - offset) > 0) {
+ if (elt && sg_is_last(&sg[elt - 1]))
+ return -EMSGSIZE;
+
if (copy > len)
copy = len;
elt += __skb_to_sgvec(frag_iter, sg+elt, offset - start,
@@ -3581,6 +3588,9 @@ int skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int le
{
int nsg = __skb_to_sgvec(skb, sg, offset, len);
+ if (nsg <= 0)
+ return nsg;
+
sg_mark_end(&sg[nsg - 1]);
return nsg;
--
2.12.2
^ permalink raw reply related
* [PATCH v5 3/5] rxrpc: check return value of skb_to_sgvec always
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
Cc: Jason A. Donenfeld
In-Reply-To: <20170425155215.4835-1-Jason@zx2c4.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
net/rxrpc/rxkad.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 4374e7b9c7bf..dcf46c9c3ece 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -229,7 +229,9 @@ static int rxkad_secure_packet_encrypt(const struct rxrpc_call *call,
len &= ~(call->conn->size_align - 1);
sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, 0, len);
+ err = skb_to_sgvec(skb, sg, 0, len);
+ if (unlikely(err < 0))
+ goto out;
skcipher_request_set_crypt(req, sg, sg, len, iv.x);
crypto_skcipher_encrypt(req);
@@ -342,7 +344,8 @@ static int rxkad_verify_packet_1(struct rxrpc_call *call, struct sk_buff *skb,
goto nomem;
sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, offset, 8);
+ if (unlikely(skb_to_sgvec(skb, sg, offset, 8) < 0))
+ goto nomem;
/* start the decryption afresh */
memset(&iv, 0, sizeof(iv));
@@ -429,7 +432,8 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
}
sg_init_table(sg, nsg);
- skb_to_sgvec(skb, sg, offset, len);
+ if (unlikely(skb_to_sgvec(skb, sg, offset, len) < 0))
+ goto nomem;
/* decrypt from the session key */
token = call->conn->params.key->payload.data[0];
--
2.12.2
^ permalink raw reply related
* [PATCH v5 4/5] macsec: check return value of skb_to_sgvec always
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
Cc: Jason A. Donenfeld
In-Reply-To: <20170425155215.4835-1-Jason@zx2c4.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/net/macsec.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dbab05afcdbe..d846f42b99ec 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -733,7 +733,12 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb,
macsec_fill_iv(iv, secy->sci, pn);
sg_init_table(sg, MAX_SKB_FRAGS + 1);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ ret = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(ret < 0)) {
+ macsec_txsa_put(tx_sa);
+ kfree_skb(skb);
+ return ERR_PTR(ret);
+ }
if (tx_sc->encrypt) {
int len = skb->len - macsec_hdr_len(sci_present) -
@@ -937,7 +942,11 @@ static struct sk_buff *macsec_decrypt(struct sk_buff *skb,
macsec_fill_iv(iv, sci, ntohl(hdr->packet_number));
sg_init_table(sg, MAX_SKB_FRAGS + 1);
- skb_to_sgvec(skb, sg, 0, skb->len);
+ ret = skb_to_sgvec(skb, sg, 0, skb->len);
+ if (unlikely(ret < 0)) {
+ kfree_skb(skb);
+ return ERR_PTR(ret);
+ }
if (hdr->tci_an & MACSEC_TCI_E) {
/* confidentiality: ethernet + macsec header
--
2.12.2
^ permalink raw reply related
* [PATCH v5 5/5] virtio_net: check return value of skb_to_sgvec always
From: Jason A. Donenfeld @ 2017-04-25 15:52 UTC (permalink / raw)
To: netdev, linux-kernel, David.Laight, kernel-hardening, davem
Cc: Jason A. Donenfeld
In-Reply-To: <20170425155215.4835-1-Jason@zx2c4.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/net/virtio_net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f36584616e7d..1709fd0b4bf7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1081,7 +1081,7 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
struct virtio_net_hdr_mrg_rxbuf *hdr;
const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
struct virtnet_info *vi = sq->vq->vdev->priv;
- unsigned num_sg;
+ int num_sg;
unsigned hdr_len = vi->hdr_len;
bool can_push;
@@ -1114,6 +1114,8 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
sg_set_buf(sq->sg, hdr, hdr_len);
num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
}
+ if (unlikely(num_sg < 0))
+ return num_sg;
return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
}
--
2.12.2
^ permalink raw reply related
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