| Message ID | 20190530160652.4256-2-andrea.righi@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,B] netlink: Don't shift on 64 for ngroups | expand |
Applied on other series, good test results.
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
This fixes an undesirable Bionic-specific behavior; clean cherry-pick from stable. Acked-by: Kamal Mostafa <kamal@canonical.com> -Kamal On Thu, May 30, 2019 at 06:06:52PM +0200, Andrea Righi wrote: > From: Dmitry Safonov <dima@arista.com> > > BugLink: https://bugs.launchpad.net/bugs/1831103 > > It's legal to have 64 groups for netlink_sock. > > As user-supplied nladdr->nl_groups is __u32, it's possible to subscribe > only to first 32 groups. > > The check for correctness of .bind() userspace supplied parameter > is done by applying mask made from ngroups shift. Which broke Android > as they have 64 groups and the shift for mask resulted in an overflow. > > Fixes: 61f4b23769f0 ("netlink: Don't shift with UB on nlk->ngroups") > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: netdev@vger.kernel.org > Cc: stable@vger.kernel.org > Reported-and-Tested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Dmitry Safonov <dima@arista.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 91874ecf32e41b5d86a4cb9d60e0bee50d828058) > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> > --- > net/netlink/af_netlink.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 07e61faaea47..6db2daedf01c 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -980,8 +980,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, > > if (nlk->ngroups == 0) > groups = 0; > - else > - groups &= (1ULL << nlk->ngroups) - 1; > + else if (nlk->ngroups < 8*sizeof(groups)) > + groups &= (1UL << nlk->ngroups) - 1; > > bound = nlk->bound; > if (bound) { > -- > 2.20.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
On 30/05/2019 17:06, Andrea Righi wrote: > From: Dmitry Safonov <dima@arista.com> > > BugLink: https://bugs.launchpad.net/bugs/1831103 > > It's legal to have 64 groups for netlink_sock. > > As user-supplied nladdr->nl_groups is __u32, it's possible to subscribe > only to first 32 groups. > > The check for correctness of .bind() userspace supplied parameter > is done by applying mask made from ngroups shift. Which broke Android > as they have 64 groups and the shift for mask resulted in an overflow. > > Fixes: 61f4b23769f0 ("netlink: Don't shift with UB on nlk->ngroups") > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: netdev@vger.kernel.org > Cc: stable@vger.kernel.org > Reported-and-Tested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Dmitry Safonov <dima@arista.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 91874ecf32e41b5d86a4cb9d60e0bee50d828058) > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> > --- > net/netlink/af_netlink.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 07e61faaea47..6db2daedf01c 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -980,8 +980,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, > > if (nlk->ngroups == 0) > groups = 0; > - else > - groups &= (1ULL << nlk->ngroups) - 1; > + else if (nlk->ngroups < 8*sizeof(groups)) > + groups &= (1UL << nlk->ngroups) - 1; > > bound = nlk->bound; > if (bound) { > This has successful test results and clean upstream cherry pick that has been cc'd to stable so I'm satisfied it's a good fix to a known issue with the netlink connector bind. Thanks Andrea, Acked-by: Colin Ian King <colin.king@canonical.com>
On 2019-05-30 18:06:52 , Andrea Righi wrote: > From: Dmitry Safonov <dima@arista.com> > > BugLink: https://bugs.launchpad.net/bugs/1831103 > > It's legal to have 64 groups for netlink_sock. > > As user-supplied nladdr->nl_groups is __u32, it's possible to subscribe > only to first 32 groups. > > The check for correctness of .bind() userspace supplied parameter > is done by applying mask made from ngroups shift. Which broke Android > as they have 64 groups and the shift for mask resulted in an overflow. > > Fixes: 61f4b23769f0 ("netlink: Don't shift with UB on nlk->ngroups") > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: netdev@vger.kernel.org > Cc: stable@vger.kernel.org > Reported-and-Tested-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Dmitry Safonov <dima@arista.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (cherry picked from commit 91874ecf32e41b5d86a4cb9d60e0bee50d828058) > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> > --- > net/netlink/af_netlink.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 07e61faaea47..6db2daedf01c 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -980,8 +980,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, > > if (nlk->ngroups == 0) > groups = 0; > - else > - groups &= (1ULL << nlk->ngroups) - 1; > + else if (nlk->ngroups < 8*sizeof(groups)) > + groups &= (1UL << nlk->ngroups) - 1; > > bound = nlk->bound; > if (bound) { > -- > 2.20.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 07e61faaea47..6db2daedf01c 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -980,8 +980,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, if (nlk->ngroups == 0) groups = 0; - else - groups &= (1ULL << nlk->ngroups) - 1; + else if (nlk->ngroups < 8*sizeof(groups)) + groups &= (1UL << nlk->ngroups) - 1; bound = nlk->bound; if (bound) {