| Message ID | 7187C142-99F1-4A96-9BE6-650B10C9B22D@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> case AX25_T1: > - if (opt < 1) { > + if (opt < 1 || opt > 30) { Where do these values come from ? If they are from some 'standard' then really we should avoid restricting needlessly to it, particularly as AX.25 isn't well defined and is used for all sorts of crazy stuff where the usual range of settings isn't useful. Restricting to the point it would overflow makes sense however. Alan -- 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
All these magic numbers come from net/ax25/sysctl_net_ax25.c, where min/max values of each field are set for sysctl. Is it okay to use them? - xi On Wed, Nov 23, 2011 at 5:44 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: >> case AX25_T1: >> - if (opt < 1) { >> + if (opt < 1 || opt > 30) { > > Where do these values come from ? If they are from some 'standard' then > really we should avoid restricting needlessly to it, particularly as > AX.25 isn't well defined and is used for all sorts of crazy stuff where > the usual range of settings isn't useful. > > Restricting to the point it would overflow makes sense however. > > Alan > > -- 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
On Wed, 23 Nov 2011 09:04:11 -0500 Xi Wang <xi.wang@gmail.com> wrote: > All these magic numbers come from net/ax25/sysctl_net_ax25.c, where > min/max values of each field are set for sysctl. Is it okay to use > them? The sysctl range is the 'standard' range, but it's always historically been possible to override them in apps for special cases. I'm wary of changing that because people do insane things like AX.25 bounced off the moon where you need very long timeouts. Alan -- 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
> > All these magic numbers come from net/ax25/sysctl_net_ax25.c, where > > min/max values of each field are set for sysctl. Is it okay to use > > them? > > The sysctl range is the 'standard' range, but it's always historically > been possible to override them in apps for special cases. I'm wary of > changing that because people do insane things like AX.25 > bounced off the moon where you need very long timeouts. It is a long time since I wrote any of the X.25 protocol stack layers, but I would agree that limiting timers to the values defined in the standard is probably not a good idea. Even normal telco's may have decided to use values that are outside the nominal range. These timers are almost certainly either 'guard' timers for missing responses or retransmit timers for 'keepalive' messages - so allowing much larger values doesn't matter. I'd only limit them in order to stop the code breaking. The lower limit (1 second) will be below the limit for the protocol - but exists to stop the code breaking. David -- 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
On Tue, Nov 22, 2011 at 11:28:24PM -0500, Xi Wang wrote: > ax25_setsockopt() misses several upper-bound checks on the > user-controlled value. > > > Reported-by: Fan Long <longfancn@gmail.com> > Signed-off-by: Xi Wang <xi.wang@gmail.com> > --- > net/ax25/af_ax25.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c > index e7c69f4..be6a8cf 100644 > --- a/net/ax25/af_ax25.c > +++ b/net/ax25/af_ax25.c > @@ -571,7 +571,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, > break; > > case AX25_T1: > - if (opt < 1) { > + if (opt < 1 || opt > 30) { > res = -EINVAL; > break; > } 30 seconds is definitively too low. The TCP spec assumes that a RTT of up to 120s is possible. In slow packet radio networks 15 minutes are easily possible in HF networks. How about AX.25 to the P5A mars mission? A silly value for T1 will be caught further down the road, so no damage but an application really should receive a sensible return value when trying something stupid. If an apps wants to shoot itself into the foot there is nothing wrong with being the arms dealer so an error check should be something like if (val > ULONG_MAX / HZ) bail_out; which will do the right thing even on a 64-bit system and prevent an overflow in the multiplication further down. > @@ -580,7 +580,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, > break; > > case AX25_T2: > - if (opt < 1) { > + if (opt < 1 || opt > 20) { > res = -EINVAL; > break; > } An excessive value here could result in a timer being set to expire in the past similar in effect to setting a very low value. Again it's ok if a user tries to shoot himself into the other foot as well. > @@ -596,7 +596,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, > break; > > case AX25_T3: > - if (opt < 1) { > + if (opt < 0 || opt > 3600) { > res = -EINVAL; > break; > } For a stable link it should be possible to set a very high T3 value, potencially high enough to effectively disable the T3 functionality. > @@ -604,7 +604,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, > break; > > case AX25_IDLE: > - if (opt < 0) { > + if (opt < 0 || opt > 65535) { > res = -EINVAL; > break; > } I have an updated patch which I'm testing right now. Ralf -- 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 a lot for your comments! Look forward to your patch. - xi -- 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/ax25/af_ax25.c b/net/ax25/af_ax25.c index e7c69f4..be6a8cf 100644 --- a/net/ax25/af_ax25.c +++ b/net/ax25/af_ax25.c @@ -571,7 +571,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, break; case AX25_T1: - if (opt < 1) { + if (opt < 1 || opt > 30) { res = -EINVAL; break; } @@ -580,7 +580,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, break; case AX25_T2: - if (opt < 1) { + if (opt < 1 || opt > 20) { res = -EINVAL; break; } @@ -596,7 +596,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, break; case AX25_T3: - if (opt < 1) { + if (opt < 0 || opt > 3600) { res = -EINVAL; break; } @@ -604,7 +604,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname, break; case AX25_IDLE: - if (opt < 0) { + if (opt < 0 || opt > 65535) { res = -EINVAL; break; }
ax25_setsockopt() misses several upper-bound checks on the user-controlled value. Reported-by: Fan Long <longfancn@gmail.com> Signed-off-by: Xi Wang <xi.wang@gmail.com> --- net/ax25/af_ax25.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)