| Message ID | CA+nUz_J73agjPQPqdbTraUPLyv39sqQ4kvZTmrZrFXLW1faqwg@mail.gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 04 octobre 2011 à 14:00 +0000, Matthew Daley a écrit : > X.25 call user data is being copied in its entirety from incoming messages > without consideration to the size of the destination buffers, leading to > possible buffer overflows. Validate incoming call user data lengths before > these copies are performed. > > It appears this issue was noticed some time ago, however nothing seemed to > come of it: see http://www.spinics.net/lists/linux-x25/msg00043.html and > commit 8db09f26f912f7c90c764806e804b558da520d4f. > > Signed-off-by: Matthew Daley <mattjd@gmail.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Cc: Andrew Hendry <andrew.hendry@gmail.com> > Cc: stable <stable@kernel.org> > --- > Hopefully this is acceptable; I thought someone with greater knowledge of > X.25 might step in :) It should take into account Andrew's suggestion to > move af_x25.c's check to before the call acception. > > net/x25/af_x25.c | 6 ++++++ > net/x25/x25_in.c | 3 +++ > 2 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c > index d306154..a4bd172 100644 > --- a/net/x25/af_x25.c > +++ b/net/x25/af_x25.c > @@ -959,6 +959,12 @@ int x25_rx_call_request(struct sk_buff *skb, > struct x25_neigh *nb, > skb_pull(skb,len); > > /* > + * Ensure that the amount of call user data is valid. > + */ > + if (skb->len > X25_MAX_CUD_LEN) > + goto out_clear_request; > + > + /* > * Find a listener for the particular address/cud pair. > */ > sk = x25_find_listener(&source_addr,skb); > diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c > index 0b073b5..63488fd 100644 > --- a/net/x25/x25_in.c > +++ b/net/x25/x25_in.c > @@ -127,6 +127,9 @@ static int x25_state1_machine(struct sock *sk, > struct sk_buff *skb, int frametyp > * Copy any Call User Data. > */ > if (skb->len > 0) { > + if (skb->len > X25_MAX_CUD_LEN) > + goto out_clear; > + > skb_copy_from_linear_data(skb, > x25->calluserdata.cuddata, > skb->len); > -- It seems fine to me. Acked-by: Eric Dumazet <eric.dumazet@gmail.com> Now, you could also fix x25 for undersized/frags frames : Apparently, no check is done in this respect (missing pskb_may_pull() calls)... -- 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
Ran this on a few systems and put a few GB through it, looks good. Tested-by: Andrew Hendry <andrew.hendry@gmail.com> On Wed, Oct 5, 2011 at 1:33 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 04 octobre 2011 à 14:00 +0000, Matthew Daley a écrit : >> X.25 call user data is being copied in its entirety from incoming messages >> without consideration to the size of the destination buffers, leading to >> possible buffer overflows. Validate incoming call user data lengths before >> these copies are performed. >> >> It appears this issue was noticed some time ago, however nothing seemed to >> come of it: see http://www.spinics.net/lists/linux-x25/msg00043.html and >> commit 8db09f26f912f7c90c764806e804b558da520d4f. >> >> Signed-off-by: Matthew Daley <mattjd@gmail.com> >> Cc: Eric Dumazet <eric.dumazet@gmail.com> >> Cc: Andrew Hendry <andrew.hendry@gmail.com> >> Cc: stable <stable@kernel.org> >> --- >> Hopefully this is acceptable; I thought someone with greater knowledge of >> X.25 might step in :) It should take into account Andrew's suggestion to >> move af_x25.c's check to before the call acception. >> >> net/x25/af_x25.c | 6 ++++++ >> net/x25/x25_in.c | 3 +++ >> 2 files changed, 9 insertions(+), 0 deletions(-) >> >> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c >> index d306154..a4bd172 100644 >> --- a/net/x25/af_x25.c >> +++ b/net/x25/af_x25.c >> @@ -959,6 +959,12 @@ int x25_rx_call_request(struct sk_buff *skb, >> struct x25_neigh *nb, >> skb_pull(skb,len); >> >> /* >> + * Ensure that the amount of call user data is valid. >> + */ >> + if (skb->len > X25_MAX_CUD_LEN) >> + goto out_clear_request; >> + >> + /* >> * Find a listener for the particular address/cud pair. >> */ >> sk = x25_find_listener(&source_addr,skb); >> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c >> index 0b073b5..63488fd 100644 >> --- a/net/x25/x25_in.c >> +++ b/net/x25/x25_in.c >> @@ -127,6 +127,9 @@ static int x25_state1_machine(struct sock *sk, >> struct sk_buff *skb, int frametyp >> * Copy any Call User Data. >> */ >> if (skb->len > 0) { >> + if (skb->len > X25_MAX_CUD_LEN) >> + goto out_clear; >> + >> skb_copy_from_linear_data(skb, >> x25->calluserdata.cuddata, >> skb->len); >> -- > > It seems fine to me. > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> > > Now, you could also fix x25 for undersized/frags frames : > > Apparently, no check is done in this respect (missing pskb_may_pull() > calls)... > > > > -- 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/af_x25.c b/net/x25/af_x25.c index d306154..a4bd172 100644 --- a/net/x25/af_x25.c +++ b/net/x25/af_x25.c @@ -959,6 +959,12 @@ int x25_rx_call_request(struct sk_buff *skb, struct x25_neigh *nb, skb_pull(skb,len); /* + * Ensure that the amount of call user data is valid. + */ + if (skb->len > X25_MAX_CUD_LEN) + goto out_clear_request; + + /* * Find a listener for the particular address/cud pair. */ sk = x25_find_listener(&source_addr,skb); diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c index 0b073b5..63488fd 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -127,6 +127,9 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp * Copy any Call User Data. */ if (skb->len > 0) { + if (skb->len > X25_MAX_CUD_LEN) + goto out_clear; + skb_copy_from_linear_data(skb, x25->calluserdata.cuddata,
X.25 call user data is being copied in its entirety from incoming messages without consideration to the size of the destination buffers, leading to possible buffer overflows. Validate incoming call user data lengths before these copies are performed. It appears this issue was noticed some time ago, however nothing seemed to come of it: see http://www.spinics.net/lists/linux-x25/msg00043.html and commit 8db09f26f912f7c90c764806e804b558da520d4f. Signed-off-by: Matthew Daley <mattjd@gmail.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Andrew Hendry <andrew.hendry@gmail.com> Cc: stable <stable@kernel.org> --- Hopefully this is acceptable; I thought someone with greater knowledge of X.25 might step in :) It should take into account Andrew's suggestion to move af_x25.c's check to before the call acception. net/x25/af_x25.c | 6 ++++++ net/x25/x25_in.c | 3 +++ 2 files changed, 9 insertions(+), 0 deletions(-) skb->len); -- 1.7.2.5 -- 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