| Message ID | 20240306095449.1782369-1-Ilia.Gavrilov@infotecs.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] l2tp: fix incorrect parameter validation in the pppol2tp_getsockopt() function | expand |
On Wed, Mar 06, 2024 at 09:58:10 +0000, Gavrilov Ilia wrote: > diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c > index f011af6601c9..6146e4e67bbb 100644 > --- a/net/l2tp/l2tp_ppp.c > +++ b/net/l2tp/l2tp_ppp.c > @@ -1356,11 +1356,11 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname, > if (get_user(len, optlen)) > return -EFAULT; > > - len = min_t(unsigned int, len, sizeof(int)); > - > if (len < 0) > return -EINVAL; > > + len = min_t(unsigned int, len, sizeof(int)); > + > err = -ENOTCONN; > if (!sk->sk_user_data) > goto end; I think this code in l2tp_ppp.c has probably been inspired by a similar implementations elsewhere in the kernel -- for example net/ipv4/udp.c udp_lib_getsockopt does the same thing, and apparently has been that way since the dawn of git time. I note however that plenty of other getsockopt implementations which are using min_t(unsigned int, len, sizeof(int)) don't check the length value at all: as an example, net/ipv6/raw.c do_rawv6_getsockopt. As it stands right now in the l2tp_ppp.c code, I think the check on len will end up doing nothing, as you point out. So moving the len check to before the min_t() call may in theory possibly catch out (insane?) userspace code passing in negative numbers which may "work" with the current kernel code. I wonder whether its safer therefore to remove the len check altogether?
On 3/6/24 16:14, Tom Parkin wrote: > On Wed, Mar 06, 2024 at 09:58:10 +0000, Gavrilov Ilia wrote: >> diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c >> index f011af6601c9..6146e4e67bbb 100644 >> --- a/net/l2tp/l2tp_ppp.c >> +++ b/net/l2tp/l2tp_ppp.c >> @@ -1356,11 +1356,11 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname, >> if (get_user(len, optlen)) >> return -EFAULT; >> >> - len = min_t(unsigned int, len, sizeof(int)); >> - >> if (len < 0) >> return -EINVAL; >> >> + len = min_t(unsigned int, len, sizeof(int)); >> + >> err = -ENOTCONN; >> if (!sk->sk_user_data) >> goto end; > > I think this code in l2tp_ppp.c has probably been inspired by a > similar implementations elsewhere in the kernel -- for example > net/ipv4/udp.c udp_lib_getsockopt does the same thing, and apparently > has been that way since the dawn of git time. > > I note however that plenty of other getsockopt implementations which > are using min_t(unsigned int, len, sizeof(int)) don't check the length > value at all: as an example, net/ipv6/raw.c do_rawv6_getsockopt. > > As it stands right now in the l2tp_ppp.c code, I think the check on > len will end up doing nothing, as you point out. > > So moving the len check to before the min_t() call may in theory > possibly catch out (insane?) userspace code passing in negative > numbers which may "work" with the current kernel code. > > I wonder whether its safer therefore to remove the len check > altogether? Thank you for answer. In my opinion, it is better to leave the 'len' check. This way it will be easier for the user to understand where the error is.
On Wed, Mar 06, 2024 at 13:46:07 +0000, Gavrilov Ilia wrote: > On 3/6/24 16:14, Tom Parkin wrote: > > As it stands right now in the l2tp_ppp.c code, I think the check on > > len will end up doing nothing, as you point out. > > > > So moving the len check to before the min_t() call may in theory > > possibly catch out (insane?) userspace code passing in negative > > numbers which may "work" with the current kernel code. > > > > I wonder whether its safer therefore to remove the len check > > altogether? > > Thank you for answer. > > In my opinion, it is better to leave the 'len' check. This way it will > be easier for the user to understand where the error is. Fair enough. My concern was that in doing so we add a new behaviour which userspace may notice and care about, but realistically I'm probably being paranoid to imagine that any such userspace exists. Thanks for your work on l2tp_ppp.c :-) Reviewed-by: Tom Parkin <tparkin@katalix.com>
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index f011af6601c9..6146e4e67bbb 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -1356,11 +1356,11 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname, if (get_user(len, optlen)) return -EFAULT; - len = min_t(unsigned int, len, sizeof(int)); - if (len < 0) return -EINVAL; + len = min_t(unsigned int, len, sizeof(int)); + err = -ENOTCONN; if (!sk->sk_user_data) goto end;
The 'len' variable can't be negative because all 'min_t' parameters cast to unsigned int, and then the minimum one is chosen. To fix it, move the if statement higher. Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core") Signed-off-by: Gavrilov Ilia <Ilia.Gavrilov@infotecs.ru> --- net/l2tp/l2tp_ppp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)