Re: [net-next 06/12] i40e/ixgbe/igb: fail on new WoL flag setting WAKE_MAGICSECURE

[Kevin Easton]

On Wed, Nov 07, 2018 at 02:48:24PM -0800, Jeff Kirsher wrote:
> From: Todd Fujinaka 
> 
> There's a new flag for setting WoL filters that is only
> enabled on one manufacturer's NICs, and it's not ours. Fail
> with EOPNOTSUPP.
> 
> Signed-off-by: Todd Fujinaka 
> Tested-by: Andrew Bowers 
> Signed-off-by: Jeff Kirsher 
> ---
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c   | 3 ++-
>  drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 3 ++-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c 
> b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 9f8464f80783..9c1211ad2c6b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -2377,7 +2377,8 @@ static int i40e_set_wol(struct net_device *netdev, 
> struct ethtool_wolinfo *wol)
>   return -EOPNOTSUPP;
>  
>   /* only magic packet is supported */
> - if (wol->wolopts && (wol->wolopts != WAKE_MAGIC))
> + if (wol->wolopts && (wol->wolopts != WAKE_MAGIC)
> +   | (wol->wolopts != WAKE_FILTER))
>   return -EOPNOTSUPP;

This doesn't look right.  WAKE_MAGIC and WAKE_FILTER are distinct, so

(wol->wolopts != WAKE_MAGIC) | (wol->wolopts != WAKE_FILTER)

will always be 1.

It looks like the existing test in this driver was fine - it *only*
accepted wol->wolopts of either 0 or WAKE_MAGIC, it was already
rejecting everything else including WAKE_FILTER.

Suggest you drop that hunk.

- Kevin

>
>  
>   /* is this a new value? */
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c 
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 5acf3b743876..c57671068245 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2113,7 +2113,7 @@ static int igb_set_wol(struct net_device *netdev, 
> struct ethtool_wolinfo *wol)
>  {
>   struct igb_adapter *adapter = netdev_priv(netdev);
>  
> - if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE))
> + if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE | WAKE_FILTER))
>   return -EOPNOTSUPP;
>  
>   if (!(adapter->flags & IGB_FLAG_WOL_SUPPORTED))
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 732b1e6ecc43..acba067cc15a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -2206,7 +2206,8 @@ static int ixgbe_set_wol(struct net_device *netdev, 
> struct ethtool_wolinfo *wol)
>  {
>   struct ixgbe_adapter *adapter = netdev_priv(netdev);
>  
> - if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
> + if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE |
> + WAKE_FILTER))
>   return -EOPNOTSUPP;
>  
>   if (ixgbe_wol_exclusion(adapter, wol))
> -- 
> 2.19.1
> 
> 
> 

Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

[Kevin Easton]

On Mon, May 07, 2018 at 04:03:25PM +0300, Michael S. Tsirkin wrote:
> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote:
> > The struct vhost_msg within struct vhost_msg_node is copied to userspace,
> > so it should be allocated with kzalloc() to ensure all structure padding
> > is zeroed.
> > 
> > Signed-off-by: Kevin Easton <ke...@guarana.org>
> > Reported-by: syzbot+87cfa083e727a2247...@syzkaller.appspotmail.com
> > ---
> >  drivers/vhost/vhost.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index f3bd8e9..1b84dcff 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
> >  /* Create a new message. */
> >  struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type)
> >  {
> > -   struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL);
> > +   struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL);
> > if (!node)
> > return NULL;
> > node->vq = vq;
> 
> 
> Let's just init the msg though.
> 
> OK it seems this is the best we can do for now,
> we need a new feature bit to fix it for 32 bit
> userspace on 64 bit kernels.
> 
> Does the following help?

Yes, the reproducer doesn't trigger the error with that patch applied.

- Kevin

Re: [PATCH] vhost: make msg padding explicit

[Kevin Easton]

On Wed, May 02, 2018 at 05:19:27PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 02, 2018 at 10:04:46AM -0400, David Miller wrote:
> > From: "Michael S. Tsirkin" 
> > Date: Wed, 2 May 2018 16:36:37 +0300
> > 
> > > Ouch.  True - and in particular the 32 bit ABI on 64 bit kernels doesn't
> > > work at all. Hmm. It's relatively new and maybe there aren't any 32 bit
> > > users yet. Thoughts?
> > 
> > If it's been in a released kernel version, we really aren't at liberty
> > to play "maybe nobody uses this" UAPI changing games.
> > 
> > Please send me a revert.
> 
> Sent. Looking at compat mess now.

Just naming the padding field isn't sufficient anyway, there also needs to
be code in vhost_new_msg() to initialise it using the name.

- Kevin

Re: [PATCH] vhost: make msg padding explicit

[Kevin Easton]

On Tue, May 01, 2018 at 02:05:51PM -0400, David Miller wrote:
> From: "Michael S. Tsirkin" <m...@redhat.com>
> Date: Tue, 1 May 2018 20:19:19 +0300
> 
> > On Tue, May 01, 2018 at 11:28:22AM -0400, David Miller wrote:
> >> From: "Michael S. Tsirkin" <m...@redhat.com>
> >> Date: Fri, 27 Apr 2018 19:02:05 +0300
> >> 
> >> > There's a 32 bit hole just after type. It's best to
> >> > give it a name, this way compiler is forced to initialize
> >> > it with rest of the structure.
> >> > 
> >> > Reported-by: Kevin Easton <ke...@guarana.org>
> >> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> >> 
> >> Michael, will you be sending this directly to Linus or would you like
> >> me to apply it to net or net-next?
> >> 
> >> Thanks.
> > 
> > I'd prefer you to apply it for net and cc stable if possible.
> 
> Ok, applied, and added to my -stable submission queue.

Hold on, this patch changes the layout for i386 (where there is
no padding at all).  And it's part of UAPI.

- Kevin

> 

Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

[Kevin Easton]

On Fri, Apr 27, 2018 at 09:07:56PM -0400, Kevin Easton wrote:
> On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote:
> > On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote:
> > > The struct vhost_msg within struct vhost_msg_node is copied to userspace,
> > > so it should be allocated with kzalloc() to ensure all structure padding
> > > is zeroed.
> > > 
> > > Signed-off-by: Kevin Easton <ke...@guarana.org>
> > > Reported-by: syzbot+87cfa083e727a2247...@syzkaller.appspotmail.com
> > 
> > Does it help if a patch naming the padding is applied,
> > and then we init just the relevant field?
> > Just curious.
> 
> No, I don't believe that is sufficient to fix the problem.

Scratch that, somehow I missed the "..and then we init just the
relevant field" part, sorry.

There's still the padding after the vhost_iotlb_msg to consider.  It's
named in the union but I don't think that's guaranteed to be initialised
when the iotlb member of the union is used to initialise things.

> I didn't name the padding in my original patch because I wasn't sure
> if the padding actually exists on 32 bit architectures?

This might still be a concern, too?

At the end of the day, zeroing 96 bytes (the full size of vhost_msg_node)
is pretty quick.

- Kevin

Re: [PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

[Kevin Easton]

On Fri, Apr 27, 2018 at 07:05:45PM +0300, Michael S. Tsirkin wrote:
> On Fri, Apr 27, 2018 at 11:45:02AM -0400, Kevin Easton wrote:
> > The struct vhost_msg within struct vhost_msg_node is copied to userspace,
> > so it should be allocated with kzalloc() to ensure all structure padding
> > is zeroed.
> > 
> > Signed-off-by: Kevin Easton <ke...@guarana.org>
> > Reported-by: syzbot+87cfa083e727a2247...@syzkaller.appspotmail.com
> 
> Does it help if a patch naming the padding is applied,
> and then we init just the relevant field?
> Just curious.

No, I don't believe that is sufficient to fix the problem.

The structure is allocated by kmalloc(), then individual fields are
initialised.  The named adding would be forced to be initialised if
it were initialised with a struct initialiser, but that's not the case.
The compiler is free to leave padding0 with whatever junk kmalloc()
left there.

Having said that, naming the padding *does* help - technically, the
compiler is allowed to put whatever it likes in the padding every time
you modify the struct.  It really needs both.

I didn't name the padding in my original patch because I wasn't sure
if the padding actually exists on 32 bit architectures?

- Kevin

Re: [PATCH net] pppoe: check sockaddr length in pppoe_connect()

[Kevin Easton]

On Fri, Apr 27, 2018 at 05:39:06PM +0200, Guillaume Nault wrote:
> On Fri, Apr 27, 2018 at 08:23:16AM -0400, Kevin Easton wrote:
...
> > There's another bug here - pppoe_connect() should also be validating
> > sp->sa_family.  My suggested patch was going to be:
> > 
> > diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> > index 1483bc7..90eb3fd 100644
> > --- a/drivers/net/ppp/pppoe.c
> > +++ b/drivers/net/ppp/pppoe.c
> > @@ -620,6 +620,14 @@ static int pppoe_connect(struct socket *sock, struct 
> > sockaddr *uservaddr,
> > lock_sock(sk);
> >  
> > error = -EINVAL;
> > +   if (sockaddr_len < sizeof(struct sockaddr_pppox))
> > +   goto end;
> > +
> > +   error = -EAFNOSUPPORT;
> > +   if (sp->sa_family != AF_PPPOX)
> > +   goto end;
> > +
> > +   error = -EINVAL;
> > if (sp->sa_protocol != PX_PROTO_OE)
> > goto end;
> >  
> > Should I rework this on top of net.git HEAD?
> > 
> > (The same applies to pppol2tp_connect()).
> > 
> Thanks for the suggestion. But ->sa_family has never been checked.
> Therefore, it has always been possible to connect a PPPoE or L2TP
> socket with an invalid .sa_family field. I'd be surprised if there were
> implementations relying on that, but we never know (for example, an
> implementation could send this field uninitialised). By being stricter
> we'd break such programs. And we don't need this field in the
> connection process, so not checking its value doesn't harm.
> 
> I'm all for being strict and validating user-provided data as much as
> possible, but I'm afraid its too late in this case.

Doesn't the same apply to supplying a bogus sockaddr_len?

I did test the rp-pppoe plugin for pppd with this patch - it does 
correctly set both the sa_family and sockaddr_len.  Checking on
Debian's codesearch also showed that everything in that corpus
that uses PX_PROTO_OE also sets AF_PPPOX.

- Kevin

> 

[PATCH net] vhost: Use kzalloc() to allocate vhost_msg_node

[Kevin Easton]

The struct vhost_msg within struct vhost_msg_node is copied to userspace,
so it should be allocated with kzalloc() to ensure all structure padding
is zeroed.

Signed-off-by: Kevin Easton <ke...@guarana.org>
Reported-by: syzbot+87cfa083e727a2247...@syzkaller.appspotmail.com
---
 drivers/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f3bd8e9..1b84dcff 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2339,7 +2339,7 @@ EXPORT_SYMBOL_GPL(vhost_disable_notify);
 /* Create a new message. */
 struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type)
 {
-   struct vhost_msg_node *node = kmalloc(sizeof *node, GFP_KERNEL);
+   struct vhost_msg_node *node = kzalloc(sizeof *node, GFP_KERNEL);
if (!node)
return NULL;
node->vq = vq;
-- 
2.8.1

Re: [PATCH net] pppoe: check sockaddr length in pppoe_connect()

[Kevin Easton]

On Mon, Apr 23, 2018 at 04:38:27PM +0200, Guillaume Nault wrote:
> We must validate sockaddr_len, otherwise userspace can pass fewer data
> than we expect and we end up accessing invalid data.
> 
> Fixes: 224cf5ad14c0 ("ppp: Move the PPP drivers")
> Reported-by: syzbot+4f03bdf92fdf9ef5d...@syzkaller.appspotmail.com
> Signed-off-by: Guillaume Nault 
> ---
>  drivers/net/ppp/pppoe.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 1483bc7b01e1..7df07337d69c 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -620,6 +620,10 @@ static int pppoe_connect(struct socket *sock, struct 
> sockaddr *uservaddr,
>   lock_sock(sk);
>  
>   error = -EINVAL;
> +
> + if (sockaddr_len != sizeof(struct sockaddr_pppox))
> + goto end;
> +
>   if (sp->sa_protocol != PX_PROTO_OE)
>   goto end;

There's another bug here - pppoe_connect() should also be validating
sp->sa_family.  My suggested patch was going to be:

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 1483bc7..90eb3fd 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -620,6 +620,14 @@ static int pppoe_connect(struct socket *sock, struct 
sockaddr *uservaddr,
lock_sock(sk);
 
error = -EINVAL;
+   if (sockaddr_len < sizeof(struct sockaddr_pppox))
+   goto end;
+
+   error = -EAFNOSUPPORT;
+   if (sp->sa_family != AF_PPPOX)
+   goto end;
+
+   error = -EINVAL;
if (sp->sa_protocol != PX_PROTO_OE)
goto end;
 
Should I rework this on top of net.git HEAD?

(The same applies to pppol2tp_connect()).

- Kevin

Re: KASAN: slab-out-of-bounds Read in pfkey_add

[Kevin Easton]

On Mon, Apr 09, 2018 at 01:56:36AM -0400, Kevin Easton wrote:
> On Sun, Apr 08, 2018 at 09:04:33PM -0700, Eric Biggers wrote:
> ...
> > 
> > Looks like this is going to be fixed by
> > https://patchwork.kernel.org/patch/10327883/ ("af_key: Always verify length 
> > of
> > provided sadb_key"), but it's not applied yet to the ipsec tree yet.  
> > Kevin, for
> > future reference, for syzbot bugs it would be helpful to reply to the 
> > original
> > bug report and say that a patch was sent out, or even better send the patch 
> > as a
> > reply to the bug report email, e.g.
> > 
> > git format-patch 
> > --in-reply-to="<001a114292fadd3e250560706...@google.com>"
> > 
> > for this one (and the Message ID can be found in the syzkaller-bugs archive 
> > even
> > if the email isn't in your inbox).
> 
> Sure, I can do that.

I recalled one reason I _didn't_ do this - the message ID is retrievable
from the archived email, but because the archive is Google Groups the
message recipients aren't (only masked).

- Kevin

Re: [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

On Mon, Apr 09, 2018 at 12:34:42PM +0200, Steffen Klassert wrote:
> On Sat, Apr 07, 2018 at 11:40:47AM -0400, Kevin Easton wrote:
> > Several places use (x + 7) / 8 to convert from a number of bits to a number
> > of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> > with other parts of the same file.
> > 
> > Signed-off-by: Kevin Easton <ke...@guarana.org>
> 
> Please resubmit this one to ipsec-next after the
> merge window. Thanks!

Will do!

- Kevin

Re: KASAN: slab-out-of-bounds Read in pfkey_add

[Kevin Easton]

On Sun, Apr 08, 2018 at 09:04:33PM -0700, Eric Biggers wrote:
...
> 
> Looks like this is going to be fixed by
> https://patchwork.kernel.org/patch/10327883/ ("af_key: Always verify length of
> provided sadb_key"), but it's not applied yet to the ipsec tree yet.  Kevin, 
> for
> future reference, for syzbot bugs it would be helpful to reply to the original
> bug report and say that a patch was sent out, or even better send the patch 
> as a
> reply to the bug report email, e.g.
> 
>   git format-patch 
> --in-reply-to="<001a114292fadd3e250560706...@google.com>"
> 
> for this one (and the Message ID can be found in the syzkaller-bugs archive 
> even
> if the email isn't in your inbox).

Sure, I can do that.

- Kevin

[PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

Several places use (x + 7) / 8 to convert from a number of bits to a number
of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
with other parts of the same file.

Signed-off-by: Kevin Easton <ke...@guarana.org>
---
 net/key/af_key.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index e62e52e..f3ebb84 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -822,12 +822,12 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const 
struct xfrm_state *x,
if (add_keys) {
if (x->aalg && x->aalg->alg_key_len) {
auth_key_size =
-   PFKEY_ALIGN8((x->aalg->alg_key_len + 7) / 8);
+   PFKEY_ALIGN8(DIV_ROUND_UP(x->aalg->alg_key_len, 
8));
size += sizeof(struct sadb_key) + auth_key_size;
}
if (x->ealg && x->ealg->alg_key_len) {
encrypt_key_size =
-   PFKEY_ALIGN8((x->ealg->alg_key_len+7) / 8);
+   PFKEY_ALIGN8(DIV_ROUND_UP(x->ealg->alg_key_len, 
8));
size += sizeof(struct sadb_key) + encrypt_key_size;
}
}
@@ -987,7 +987,8 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct 
xfrm_state *x,
key->sadb_key_exttype = SADB_EXT_KEY_AUTH;
key->sadb_key_bits = x->aalg->alg_key_len;
key->sadb_key_reserved = 0;
-   memcpy(key + 1, x->aalg->alg_key, (x->aalg->alg_key_len+7)/8);
+   memcpy(key + 1, x->aalg->alg_key,
+   DIV_ROUND_UP(x->aalg->alg_key_len, 8));
}
/* encrypt key */
if (add_keys && encrypt_key_size) {
@@ -998,7 +999,7 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct 
xfrm_state *x,
key->sadb_key_bits = x->ealg->alg_key_len;
key->sadb_key_reserved = 0;
memcpy(key + 1, x->ealg->alg_key,
-  (x->ealg->alg_key_len+7)/8);
+   DIV_ROUND_UP(x->ealg->alg_key_len, 8));
}
 
/* sa */
@@ -1193,7 +1194,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
goto out;
}
if (key)
-   keysize = (key->sadb_key_bits + 7) / 8;
+   keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->aalg = kmalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
if (!x->aalg) {
err = -ENOMEM;
@@ -1232,7 +1233,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
}
key = (struct sadb_key*) 
ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key)
-   keysize = (key->sadb_key_bits + 7) / 8;
+   keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->ealg = kmalloc(sizeof(*x->ealg) + keysize, 
GFP_KERNEL);
if (!x->ealg) {
err = -ENOMEM;
-- 
2.8.1

[PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun

[Kevin Easton]

As found by syzbot, af_key does not properly validate the key length in
sadb_key messages from userspace.  This can result in copying from beyond
the end of the sadb_key part of the message, or indeed beyond the end of
the entire packet.

Both these patches apply cleanly to ipsec-next.  Based on Steffen's
feedback I have re-ordered them so that the fix only is in patch 1, which
I would suggest is also a stable tree candidate, whereas patch 2 is a
cleanup only.

Kevin Easton (2):
  af_key: Always verify length of provided sadb_key
  af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

 net/key/af_key.c | 58 
 1 file changed, 42 insertions(+), 16 deletions(-)

-- 
2.8.1

[PATCH v2 1/2] af_key: Always verify length of provided sadb_key

[Kevin Easton]

Key extensions (struct sadb_key) include a user-specified number of key
bits.  The kernel uses that number to determine how much key data to copy
out of the message in pfkey_msg2xfrm_state().

The length of the sadb_key message must be verified to be long enough,
even in the case of SADB_X_AALG_NULL.  Furthermore, the sadb_key_len value
must be long enough to include both the key data and the struct sadb_key
itself.

Introduce a helper function verify_key_len(), and call it from
parse_exthdrs() where other exthdr types are similarly checked for
correctness.

Signed-off-by: Kevin Easton <ke...@guarana.org>
Reported-by: 
syzbot+5022a34ca5a3d49b84223653fab632dfb7b4c...@syzkaller.appspotmail.com
---
 net/key/af_key.c | 45 +++--
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 7e2e718..e62e52e 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -437,6 +437,24 @@ static int verify_address_len(const void *p)
return 0;
 }
 
+static inline int sadb_key_len(const struct sadb_key *key)
+{
+   int key_bytes = DIV_ROUND_UP(key->sadb_key_bits, 8);
+
+   return DIV_ROUND_UP(sizeof(struct sadb_key) + key_bytes,
+   sizeof(uint64_t));
+}
+
+static int verify_key_len(const void *p)
+{
+   const struct sadb_key *key = p;
+
+   if (sadb_key_len(key) > key->sadb_key_len)
+   return -EINVAL;
+
+   return 0;
+}
+
 static inline int pfkey_sec_ctx_len(const struct sadb_x_sec_ctx *sec_ctx)
 {
return DIV_ROUND_UP(sizeof(struct sadb_x_sec_ctx) +
@@ -533,16 +551,25 @@ static int parse_exthdrs(struct sk_buff *skb, const 
struct sadb_msg *hdr, void *
return -EINVAL;
if (ext_hdrs[ext_type-1] != NULL)
return -EINVAL;
-   if (ext_type == SADB_EXT_ADDRESS_SRC ||
-   ext_type == SADB_EXT_ADDRESS_DST ||
-   ext_type == SADB_EXT_ADDRESS_PROXY ||
-   ext_type == SADB_X_EXT_NAT_T_OA) {
+   switch (ext_type) {
+   case SADB_EXT_ADDRESS_SRC:
+   case SADB_EXT_ADDRESS_DST:
+   case SADB_EXT_ADDRESS_PROXY:
+   case SADB_X_EXT_NAT_T_OA:
if (verify_address_len(p))
return -EINVAL;
-   }
-   if (ext_type == SADB_X_EXT_SEC_CTX) {
+   break;
+   case SADB_X_EXT_SEC_CTX:
if (verify_sec_ctx_len(p))
return -EINVAL;
+   break;
+   case SADB_EXT_KEY_AUTH:
+   case SADB_EXT_KEY_ENCRYPT:
+   if (verify_key_len(p))
+   return -EINVAL;
+   break;
+   default:
+   break;
}
ext_hdrs[ext_type-1] = (void *) p;
}
@@ -1104,14 +1131,12 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
if (key != NULL &&
sa->sadb_sa_auth != SADB_X_AALG_NULL &&
-   ((key->sadb_key_bits+7) / 8 == 0 ||
-(key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+   key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key != NULL &&
sa->sadb_sa_encrypt != SADB_EALG_NULL &&
-   ((key->sadb_key_bits+7) / 8 == 0 ||
-(key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+   key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
 
x = xfrm_state_alloc(net);
-- 
2.8.1

Re: [PATCH 1/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

On Wed, Mar 28, 2018 at 07:59:25AM +0200, Steffen Klassert wrote:
> On Mon, Mar 26, 2018 at 07:39:16AM -0400, Kevin Easton wrote:
> > Several places use (x + 7) / 8 to convert from a number of bits to a number
> > of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> > with other parts of the same file.
> > 
> > Signed-off-by: Kevin Easton <ke...@guarana.org>
> 
> Is this a fix or just a cleanup?
> If it is just a cleanup, please resent it based on ipsec-next.

Hi Steffen,

This patch (1/2) is just a cleanup, but it's in preparation for patch 2/2
which is a fix and modifies some of the same lines.

- Kevin

[PATCH 1/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent

[Kevin Easton]

Several places use (x + 7) / 8 to convert from a number of bits to a number
of bytes.  Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
with other parts of the same file.

Signed-off-by: Kevin Easton <ke...@guarana.org>
---
 net/key/af_key.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 7e2e718..911b68d 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -795,12 +795,12 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const 
struct xfrm_state *x,
if (add_keys) {
if (x->aalg && x->aalg->alg_key_len) {
auth_key_size =
-   PFKEY_ALIGN8((x->aalg->alg_key_len + 7) / 8);
+   PFKEY_ALIGN8(DIV_ROUND_UP(x->aalg->alg_key_len, 
8));
size += sizeof(struct sadb_key) + auth_key_size;
}
if (x->ealg && x->ealg->alg_key_len) {
encrypt_key_size =
-   PFKEY_ALIGN8((x->ealg->alg_key_len+7) / 8);
+   PFKEY_ALIGN8(DIV_ROUND_UP(x->ealg->alg_key_len, 
8));
size += sizeof(struct sadb_key) + encrypt_key_size;
}
}
@@ -960,7 +960,8 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct 
xfrm_state *x,
key->sadb_key_exttype = SADB_EXT_KEY_AUTH;
key->sadb_key_bits = x->aalg->alg_key_len;
key->sadb_key_reserved = 0;
-   memcpy(key + 1, x->aalg->alg_key, (x->aalg->alg_key_len+7)/8);
+   memcpy(key + 1, x->aalg->alg_key,
+   DIV_ROUND_UP(x->aalg->alg_key_len, 8));
}
/* encrypt key */
if (add_keys && encrypt_key_size) {
@@ -971,7 +972,7 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct 
xfrm_state *x,
key->sadb_key_bits = x->ealg->alg_key_len;
key->sadb_key_reserved = 0;
memcpy(key + 1, x->ealg->alg_key,
-  (x->ealg->alg_key_len+7)/8);
+   DIV_ROUND_UP(x->ealg->alg_key_len, 8));
}
 
/* sa */
@@ -1104,14 +1105,14 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
if (key != NULL &&
sa->sadb_sa_auth != SADB_X_AALG_NULL &&
-   ((key->sadb_key_bits+7) / 8 == 0 ||
-(key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+   (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
+DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * 
sizeof(uint64_t)))
return ERR_PTR(-EINVAL);
key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key != NULL &&
sa->sadb_sa_encrypt != SADB_EALG_NULL &&
-   ((key->sadb_key_bits+7) / 8 == 0 ||
-(key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+   (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
+DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * 
sizeof(uint64_t)))
return ERR_PTR(-EINVAL);
 
x = xfrm_state_alloc(net);
@@ -1168,7 +1169,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
goto out;
}
if (key)
-   keysize = (key->sadb_key_bits + 7) / 8;
+   keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->aalg = kmalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
if (!x->aalg) {
err = -ENOMEM;
@@ -1207,7 +1208,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
}
key = (struct sadb_key*) 
ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key)
-   keysize = (key->sadb_key_bits + 7) / 8;
+   keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->ealg = kmalloc(sizeof(*x->ealg) + keysize, 
GFP_KERNEL);
if (!x->ealg) {
err = -ENOMEM;
-- 
2.8.1

[PATCH 2/2] af_key: Always verify length of provided sadb_key

[Kevin Easton]

Key extensions (struct sadb_key) include a user-specified number of key
bits.  The kernel uses that number to determine how much key data to copy
out of the message in pfkey_msg2xfrm_state().

The length of the sadb_key message must be verified to be long enough,
even in the case of SADB_X_AALG_NULL.  Furthermore, the sadb_key_len value
must be long enough to include both the key data and the struct sadb_key
itself.

Introduce a helper function verify_key_len(), and call it from
parse_exthdrs() where other exthdr types are similarly checked for
correctness.

Signed-off-by: Kevin Easton <ke...@guarana.org>
Reported-by: 
syzbot+5022a34ca5a3d49b84223653fab632dfb7b4c...@syzkaller.appspotmail.com
---
 net/key/af_key.c | 45 +++--
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 911b68d..f3ebb84 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -437,6 +437,24 @@ static int verify_address_len(const void *p)
return 0;
 }
 
+static inline int sadb_key_len(const struct sadb_key *key)
+{
+   int key_bytes = DIV_ROUND_UP(key->sadb_key_bits, 8);
+
+   return DIV_ROUND_UP(sizeof(struct sadb_key) + key_bytes,
+   sizeof(uint64_t));
+}
+
+static int verify_key_len(const void *p)
+{
+   const struct sadb_key *key = p;
+
+   if (sadb_key_len(key) > key->sadb_key_len)
+   return -EINVAL;
+
+   return 0;
+}
+
 static inline int pfkey_sec_ctx_len(const struct sadb_x_sec_ctx *sec_ctx)
 {
return DIV_ROUND_UP(sizeof(struct sadb_x_sec_ctx) +
@@ -533,16 +551,25 @@ static int parse_exthdrs(struct sk_buff *skb, const 
struct sadb_msg *hdr, void *
return -EINVAL;
if (ext_hdrs[ext_type-1] != NULL)
return -EINVAL;
-   if (ext_type == SADB_EXT_ADDRESS_SRC ||
-   ext_type == SADB_EXT_ADDRESS_DST ||
-   ext_type == SADB_EXT_ADDRESS_PROXY ||
-   ext_type == SADB_X_EXT_NAT_T_OA) {
+   switch (ext_type) {
+   case SADB_EXT_ADDRESS_SRC:
+   case SADB_EXT_ADDRESS_DST:
+   case SADB_EXT_ADDRESS_PROXY:
+   case SADB_X_EXT_NAT_T_OA:
if (verify_address_len(p))
return -EINVAL;
-   }
-   if (ext_type == SADB_X_EXT_SEC_CTX) {
+   break;
+   case SADB_X_EXT_SEC_CTX:
if (verify_sec_ctx_len(p))
return -EINVAL;
+   break;
+   case SADB_EXT_KEY_AUTH:
+   case SADB_EXT_KEY_ENCRYPT:
+   if (verify_key_len(p))
+   return -EINVAL;
+   break;
+   default:
+   break;
}
ext_hdrs[ext_type-1] = (void *) p;
}
@@ -1105,14 +1132,12 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct 
net *net,
key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
if (key != NULL &&
sa->sadb_sa_auth != SADB_X_AALG_NULL &&
-   (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
-DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * 
sizeof(uint64_t)))
+   key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key != NULL &&
sa->sadb_sa_encrypt != SADB_EALG_NULL &&
-   (DIV_ROUND_UP(key->sadb_key_bits, 8) == 0 ||
-DIV_ROUND_UP(key->sadb_key_bits, 8) > key->sadb_key_len * 
sizeof(uint64_t)))
+   key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
 
x = xfrm_state_alloc(net);
-- 
2.8.1

[PATCH 0/2] af_key: Fix for sadb_key memcpy read overrun

[Kevin Easton]

As found by syzbot, af_key does not properly validate the key length in
sadb_key messages from userspace.  This can result in copying from beyond
the end of the sadb_key part of the message, or indeed beyond the end of
the entire packet.

Kevin Easton (2):
  af_key: Use DIV_ROUND_UP() instead of open-coded equivalent
  af_key: Always verify length of provided sadb_key

 net/key/af_key.c | 58 
 1 file changed, 42 insertions(+), 16 deletions(-)

-- 
2.8.1

Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning

[Kevin Easton]

On Fri, Jul 14, 2017 at 12:37:05PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 14, 2017 at 12:08 PM, Joe Perches  wrote:
> > On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote:
> >> We test whether a bit is set in a mask here, which is correct
> >> but gcc warns about it as it thinks it might be confusing:
> >>
> >> drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants 
> >> in boolean context, the expression will always evaluate to 'true' 
> >> [-Werror=int-in-bool-context]

...

> > Perhaps this is a logic defect and should be:
> >
> > if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : 
> > ISDNLOOP_FLAGS_B1ACTIVE)))
> 
> Yes, good catch. I had thought about it for a bit whether that would be
> the answer, but come to the wrong conclusion on my own.
> 
> Note that the version you suggested will still have the warning, so I think
> it needs to be

It shouldn't - the warning is for using an integer *constant* in boolean
context, but the result of & isn't a constant and should be fine.

!(flags & mask) is a very common idiom.

- Kevin