Message ID | 1289515414.5167.15.camel@dan |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Tested ok, although I noticed the CLASS_D default: message tries to print the facility type, length and 4 values, while it may only have 2 values. Just needs p[4] and p[5] taken out. Do you want me to spin a secondary patch or do you want to put them together? Acked-by: Andrew Hendry <andrew.hendry@gmail.com> On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote: > Sorry I didn't catch this at the same time as my previous report so it > could be included in one patch. > > On parsing malformed X.25 facilities, decrementing the remaining length > may cause it to underflow. Since the length is an unsigned integer, > this will result in the loop continuing until the kernel crashes. > > This patch adds checks to ensure decrementing the remaining length does > not cause it to wrap around. > > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> > CC: stable <stable@kernel.org> > --- > net/x25/x25_facilities.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c > index 3a8c4c4..bef8330 100644 > --- a/net/x25/x25_facilities.c > +++ b/net/x25/x25_facilities.c > @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > while (len > 0) { > switch (*p & X25_FAC_CLASS_MASK) { > case X25_FAC_CLASS_A: > + if (len < 2) > + return 0; > switch (*p) { > case X25_FAC_REVERSE: > if((p[1] & 0x81) == 0x81) { > @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > len -= 2; > break; > case X25_FAC_CLASS_B: > + if (len < 3) > + return 0; > switch (*p) { > case X25_FAC_PACKET_SIZE: > facilities->pacsize_in = p[1]; > @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > len -= 3; > break; > case X25_FAC_CLASS_C: > + if (len < 4) > + return 0; > printk(KERN_DEBUG "X.25: unknown facility %02X, " > "values %02X, %02X, %02X\n", > p[0], p[1], p[2], p[3]); > @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > len -= 4; > break; > case X25_FAC_CLASS_D: > + if (len < p[1] + 2) > + return 0; > switch (*p) { > case X25_FAC_CALLING_AE: > if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) > > > -- 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
I can take care of it. Do you want me to just remove those items from the printk statement? -Dan On Fri, 2010-11-12 at 10:24 +1100, Andrew Hendry wrote: > Tested ok, although I noticed the CLASS_D default: message tries to > print the facility type, length and 4 values, while it may only have 2 > values. Just needs p[4] and p[5] taken out. Do you want me to spin a > secondary patch or do you want to put them together? > > Acked-by: Andrew Hendry <andrew.hendry@gmail.com> > > On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote: > > Sorry I didn't catch this at the same time as my previous report so it > > could be included in one patch. > > > > On parsing malformed X.25 facilities, decrementing the remaining length > > may cause it to underflow. Since the length is an unsigned integer, > > this will result in the loop continuing until the kernel crashes. > > > > This patch adds checks to ensure decrementing the remaining length does > > not cause it to wrap around. > > > > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> > > CC: stable <stable@kernel.org> > > --- > > net/x25/x25_facilities.c | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c > > index 3a8c4c4..bef8330 100644 > > --- a/net/x25/x25_facilities.c > > +++ b/net/x25/x25_facilities.c > > @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > > while (len > 0) { > > switch (*p & X25_FAC_CLASS_MASK) { > > case X25_FAC_CLASS_A: > > + if (len < 2) > > + return 0; > > switch (*p) { > > case X25_FAC_REVERSE: > > if((p[1] & 0x81) == 0x81) { > > @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > > len -= 2; > > break; > > case X25_FAC_CLASS_B: > > + if (len < 3) > > + return 0; > > switch (*p) { > > case X25_FAC_PACKET_SIZE: > > facilities->pacsize_in = p[1]; > > @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > > len -= 3; > > break; > > case X25_FAC_CLASS_C: > > + if (len < 4) > > + return 0; > > printk(KERN_DEBUG "X.25: unknown facility %02X, " > > "values %02X, %02X, %02X\n", > > p[0], p[1], p[2], p[3]); > > @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, > > len -= 4; > > break; > > case X25_FAC_CLASS_D: > > + if (len < p[1] + 2) > > + return 0; > > switch (*p) { > > case X25_FAC_CALLING_AE: > > if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) > > > > > > -- 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
Thanks, yes just remove them from the printk. On Fri, Nov 12, 2010 at 10:49 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote: > I can take care of it. Do you want me to just remove those items from > the printk statement? > > -Dan > > On Fri, 2010-11-12 at 10:24 +1100, Andrew Hendry wrote: >> Tested ok, although I noticed the CLASS_D default: message tries to >> print the facility type, length and 4 values, while it may only have 2 >> values. Just needs p[4] and p[5] taken out. Do you want me to spin a >> secondary patch or do you want to put them together? >> >> Acked-by: Andrew Hendry <andrew.hendry@gmail.com> >> >> On Fri, Nov 12, 2010 at 9:43 AM, Dan Rosenberg <drosenberg@vsecurity.com> wrote: >> > Sorry I didn't catch this at the same time as my previous report so it >> > could be included in one patch. >> > >> > On parsing malformed X.25 facilities, decrementing the remaining length >> > may cause it to underflow. Since the length is an unsigned integer, >> > this will result in the loop continuing until the kernel crashes. >> > >> > This patch adds checks to ensure decrementing the remaining length does >> > not cause it to wrap around. >> > >> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> >> > CC: stable <stable@kernel.org> >> > --- >> > net/x25/x25_facilities.c | 8 ++++++++ >> > 1 files changed, 8 insertions(+), 0 deletions(-) >> > >> > diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c >> > index 3a8c4c4..bef8330 100644 >> > --- a/net/x25/x25_facilities.c >> > +++ b/net/x25/x25_facilities.c >> > @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, >> > while (len > 0) { >> > switch (*p & X25_FAC_CLASS_MASK) { >> > case X25_FAC_CLASS_A: >> > + if (len < 2) >> > + return 0; >> > switch (*p) { >> > case X25_FAC_REVERSE: >> > if((p[1] & 0x81) == 0x81) { >> > @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, >> > len -= 2; >> > break; >> > case X25_FAC_CLASS_B: >> > + if (len < 3) >> > + return 0; >> > switch (*p) { >> > case X25_FAC_PACKET_SIZE: >> > facilities->pacsize_in = p[1]; >> > @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, >> > len -= 3; >> > break; >> > case X25_FAC_CLASS_C: >> > + if (len < 4) >> > + return 0; >> > printk(KERN_DEBUG "X.25: unknown facility %02X, " >> > "values %02X, %02X, %02X\n", >> > p[0], p[1], p[2], p[3]); >> > @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, >> > len -= 4; >> > break; >> > case X25_FAC_CLASS_D: >> > + if (len < p[1] + 2) >> > + return 0; >> > switch (*p) { >> > case X25_FAC_CALLING_AE: >> > if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) >> > >> > >> > > > > -- 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/x25/x25_facilities.c b/net/x25/x25_facilities.c index 3a8c4c4..bef8330 100644 --- a/net/x25/x25_facilities.c +++ b/net/x25/x25_facilities.c @@ -61,6 +61,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, while (len > 0) { switch (*p & X25_FAC_CLASS_MASK) { case X25_FAC_CLASS_A: + if (len < 2) + return 0; switch (*p) { case X25_FAC_REVERSE: if((p[1] & 0x81) == 0x81) { @@ -104,6 +106,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, len -= 2; break; case X25_FAC_CLASS_B: + if (len < 3) + return 0; switch (*p) { case X25_FAC_PACKET_SIZE: facilities->pacsize_in = p[1]; @@ -125,6 +129,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, len -= 3; break; case X25_FAC_CLASS_C: + if (len < 4) + return 0; printk(KERN_DEBUG "X.25: unknown facility %02X, " "values %02X, %02X, %02X\n", p[0], p[1], p[2], p[3]); @@ -132,6 +138,8 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, len -= 4; break; case X25_FAC_CLASS_D: + if (len < p[1] + 2) + return 0; switch (*p) { case X25_FAC_CALLING_AE: if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
Sorry I didn't catch this at the same time as my previous report so it could be included in one patch. On parsing malformed X.25 facilities, decrementing the remaining length may cause it to underflow. Since the length is an unsigned integer, this will result in the loop continuing until the kernel crashes. This patch adds checks to ensure decrementing the remaining length does not cause it to wrap around. Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com> CC: stable <stable@kernel.org> --- net/x25/x25_facilities.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) -- 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