| Message ID | 20100303064001.GB2648@psychotron.redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mercredi 03 mars 2010 à 07:40 +0100, Jiri Pirko a écrit : > Subject: [net-2.6 PATCH] af_packet: move strict addr_len check right before dev_[mc/unicast]_[add/del] > > My previous patch 914c8ad2d18b62ad1420f518c0cab0b0b90ab308 incorrectly changed > the length check in packet_mc_add to be more strict. The problem is that > userspace is not filling this field (and it stays zeroed) in case of setting > PACKET_MR_PROMISC or PACKET_MR_ALLMULTI. So move the strict check to the point > in path where the addr_len must be set correctly. > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> > I am not sure it solves Pavel Roskin concern, but some credit should be given to him :) Reported-by: Pavel Roskin <proski@gnu.org> Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Jiri Pirko <jpirko@redhat.com>: > @@ -1734,7 +1738,7 @@ static int packet_mc_add(struct sock *sk, > struct packet_mreq_max *mreq) > goto done; > > err = -EINVAL; > - if (mreq->mr_alen != dev->addr_len) > + if (mreq->mr_alen > dev->addr_len) > goto done; > > err = -ENOBUFS; The patch looks good, but did you mean to include this change? It's not described.
Wed, Mar 03, 2010 at 08:01:10AM CET, proski@gnu.org wrote: >Quoting Jiri Pirko <jpirko@redhat.com>: > >>@@ -1734,7 +1738,7 @@ static int packet_mc_add(struct sock *sk, >>struct packet_mreq_max *mreq) >> goto done; >> >> err = -EINVAL; >>- if (mreq->mr_alen != dev->addr_len) >>+ if (mreq->mr_alen > dev->addr_len) >> goto done; >> >> err = -ENOBUFS; > >The patch looks good, but did you mean to include this change? It's >not described. Sure - this is revert of the bit from my original patch. I think it's clear from description. Jirka > >-- >Regards, >Pavel Roskin -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Mar 03, 2010 at 07:40:01AM CET, jpirko@redhat.com wrote: >Subject: [net-2.6 PATCH] af_packet: move strict addr_len check right before dev_[mc/unicast]_[add/del] Dave please apply this against net-next-2.6. I see that 914c8ad2d18b is still not in net-2.6. Thanks a lot Jirka > >My previous patch 914c8ad2d18b62ad1420f518c0cab0b0b90ab308 incorrectly changed >the length check in packet_mc_add to be more strict. The problem is that >userspace is not filling this field (and it stays zeroed) in case of setting >PACKET_MR_PROMISC or PACKET_MR_ALLMULTI. So move the strict check to the point >in path where the addr_len must be set correctly. > >Signed-off-by: Jiri Pirko <jpirko@redhat.com> > >diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >index 031a5e6..1612d41 100644 >--- a/net/packet/af_packet.c >+++ b/net/packet/af_packet.c >@@ -1688,6 +1688,8 @@ static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i, > { > switch (i->type) { > case PACKET_MR_MULTICAST: >+ if (i->alen != dev->addr_len) >+ return -EINVAL; > if (what > 0) > return dev_mc_add(dev, i->addr, i->alen, 0); > else >@@ -1700,6 +1702,8 @@ static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i, > return dev_set_allmulti(dev, what); > break; > case PACKET_MR_UNICAST: >+ if (i->alen != dev->addr_len) >+ return -EINVAL; > if (what > 0) > return dev_unicast_add(dev, i->addr); > else >@@ -1734,7 +1738,7 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) > goto done; > > err = -EINVAL; >- if (mreq->mr_alen != dev->addr_len) >+ if (mreq->mr_alen > dev->addr_len) > goto done; > > err = -ENOBUFS; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jiri Pirko <jpirko@redhat.com> Date: Wed, 3 Mar 2010 10:05:13 +0100 > Wed, Mar 03, 2010 at 07:40:01AM CET, jpirko@redhat.com wrote: >>Subject: [net-2.6 PATCH] af_packet: move strict addr_len check right before dev_[mc/unicast]_[add/del] > > Dave please apply this against net-next-2.6. I see that 914c8ad2d18b is still not in > net-2.6. Would you relax? I just rebased net-2.6 and net-next-2.6 after Linus took in my changes and I just applied your regression fix to my tree and was about to let you know I applied it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jiri Pirko <jpirko@redhat.com> Date: Wed, 3 Mar 2010 07:40:01 +0100 > Subject: [net-2.6 PATCH] af_packet: move strict addr_len check right before dev_[mc/unicast]_[add/del] > > My previous patch 914c8ad2d18b62ad1420f518c0cab0b0b90ab308 incorrectly changed > the length check in packet_mc_add to be more strict. The problem is that > userspace is not filling this field (and it stays zeroed) in case of setting > PACKET_MR_PROMISC or PACKET_MR_ALLMULTI. So move the strict check to the point > in path where the addr_len must be set correctly. > > Signed-off-by: Jiri Pirko <jpirko@redhat.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 031a5e6..1612d41 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1688,6 +1688,8 @@ static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i, { switch (i->type) { case PACKET_MR_MULTICAST: + if (i->alen != dev->addr_len) + return -EINVAL; if (what > 0) return dev_mc_add(dev, i->addr, i->alen, 0); else @@ -1700,6 +1702,8 @@ static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i, return dev_set_allmulti(dev, what); break; case PACKET_MR_UNICAST: + if (i->alen != dev->addr_len) + return -EINVAL; if (what > 0) return dev_unicast_add(dev, i->addr); else @@ -1734,7 +1738,7 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) goto done; err = -EINVAL; - if (mreq->mr_alen != dev->addr_len) + if (mreq->mr_alen > dev->addr_len) goto done; err = -ENOBUFS;
Subject: [net-2.6 PATCH] af_packet: move strict addr_len check right before dev_[mc/unicast]_[add/del] My previous patch 914c8ad2d18b62ad1420f518c0cab0b0b90ab308 incorrectly changed the length check in packet_mc_add to be more strict. The problem is that userspace is not filling this field (and it stays zeroed) in case of setting PACKET_MR_PROMISC or PACKET_MR_ALLMULTI. So move the strict check to the point in path where the addr_len must be set correctly. Signed-off-by: Jiri Pirko <jpirko@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html