On Fri, Apr 03, 2020 at 11:07:48AM +0200, Martin Pieuchot wrote: > On 02/04/20(Thu) 13:44, Vitaliy Makkoveev wrote: > > pipex(4) has pipex_ioctl() interface for pipex_session related routines. > > pipex_ioctl() calls should be done with pipex_iface_contex, so any > > operations should be done with pipex_sessions owned by passed > > pipex_iface_contex. pipex_session check ownership is missing within > > pipex_ioctl() so anybody can do pipex_ioctl() commands PIPEXDSESSION, > > PIPEXCSESSION, PIPEXGSTAT and PIPEXGCLOSED on any pipex_session. > > PIPEXDSESSION on foreign pppx(4) owned pipex_session will crash kernel. > > Code to reproduce and screenshot attached below. Diffs below add > > pipes_session ownrship check to pipex_ioctl() internals. > > Awesome bug report! Fix makes sense to me. > > To avoid check duplication would it make sense to have pppxioctl() call > pipex_ioctl() for a subset of ioctl(2) values?
It makes sense, but not now. pppxioctl() should find pipex_iface_context associated with pppx_if and only then call pipex_ioctl(). For PIPEXASESSION command pipex_iface_context should be created and for PIPEXDSESSION it should be deleted outside pipex(4). Current pipex(4) design is: one pipex_iface_context, one ifnet and many pipex_sessions associated with this pipex_iface_context use this one ifnet. This is ok for pppac(4), but for pppx(4) it requres to have one pipex_iface_context per each pppx_if that's overkilling. I have an idea to move ifnet out from pipex_iface_context and put it directly (really with some wrapper) to pipex_session. So pipex_sessions will have individual ifnet (for pppx(4)) or share it (for pppac(4)). pppx_dev will have only one pipex_iface_context for all its pppx_ifs. After this step PIPEXCSESSION, PIPEXGSTAT and PIPEXGCLOSED can be executed by pipex_ioctl() directly. The next step I suggest is to destroy pppx_if context within ifnet's detach hook. pipex_session will call ifnet_detach() by itself and clear pppx_if. After this step PIPEXDSESSION calls will be allowed directly on pipex_iface_context and in-kernel timeout feature will be restored. Also we have PIPEXSIFDESCR command which is not supported by pipex_ioctl() :) But at first, I suggest to add to pppx_add_session() and to pipex_add_session() check: is the session with given req->pr_protocol and req->pr_session_id already exist. At second, return to https://marc.info/?t=158540788400001&r=1&w=2 and finish cleanups and deduplication. > > > ---- cut begin > > > > #include <sys/types.h> > > #include <sys/ioctl.h> > > #include <sys/socket.h> > > #include <sys/stat.h> > > #include <stdio.h> > > #include <err.h> > > #include <fcntl.h> > > #include <string.h> > > #include <unistd.h> > > > > #include <arpa/inet.h> > > #include <netinet/in.h> > > #include <net/if.h> > > #include <net/pipex.h> > > > > #define PIPEX_CLOSE_TIMEOUT 30 > > > > int main(void) > > { > > int fd_pppx, fd_pppac; > > struct pipex_session_req reqa; > > struct pipex_session_close_req reqd; > > > > if((fd_pppx=open("/dev/pppx0", O_RDWR))<0){ > > err(1, "open(pppx0)"); > > } > > > > if((fd_pppac=open("/dev/pppac0", O_RDWR))<0){ > > err(1, "open(pppac0)"); > > } > > > > memset(&reqa, 0, sizeof(reqa)); > > reqa.pr_protocol=PIPEX_PROTO_L2TP; > > reqa.pr_peer_mru=1024; > > reqa.pr_local_address.ss_family=AF_INET; > > reqa.pr_local_address.ss_len=sizeof(struct sockaddr_in); > > reqa.pr_peer_address.ss_family=AF_INET; > > reqa.pr_peer_address.ss_len=sizeof(struct sockaddr_in); > > inet_aton("10.0.0.1", &reqa.pr_ip_srcaddr); > > inet_aton("10.0.0.2", &reqa.pr_ip_address); > > inet_aton("255.255.255.255", &reqa.pr_ip_netmask); > > > > if(ioctl(fd_pppx, PIPEXASESSION, &reqa)<0){ > > err(1, "ioctl(fd_pppx, PIPEXASESSION)"); > > } > > > > memset(&reqd, 0, sizeof(reqd)); > > reqd.pcr_protocol=PIPEX_PROTO_L2TP; > > > > if(ioctl(fd_pppac, PIPEXDSESSION, &reqd)<0){ > > err(1, "ioctl(fd_ppaac, PIPEDSESSION)"); > > } > > > > sleep(PIPEX_CLOSE_TIMEOUT+1); > > > > return 0; > > } > > > > ---- cut end > > > > This diff fix pipex_ioctl(PIPEXDSESSION) crash issue > > > > ---- cut begin > > diff --git sys/net/pipex.c sys/net/pipex.c > > index 3da8ed8..22edce3 100644 > > --- sys/net/pipex.c > > +++ sys/net/pipex.c > > @@ -230,7 +230,7 @@ pipex_ioctl(struct pipex_iface_context *pipex_iface, > > u_long cmd, caddr_t data) > > > > case PIPEXDSESSION: > > ret = pipex_close_session( > > - (struct pipex_session_close_req *)data); > > + (struct pipex_session_close_req *)data, pipex_iface); > > break; > > > > case PIPEXCSESSION: > > @@ -489,7 +489,8 @@ pipex_notify_close_session_all(void) > > } > > > > Static int > > -pipex_close_session(struct pipex_session_close_req *req) > > +pipex_close_session(struct pipex_session_close_req *req, > > + struct pipex_iface_context *iface) > > { > > struct pipex_session *session; > > > > @@ -498,6 +499,8 @@ pipex_close_session(struct pipex_session_close_req *req) > > req->pcr_session_id); > > if (session == NULL) > > return (EINVAL); > > + if (session->pipex_iface != iface) > > + return (EINVAL); > > > > /* remove from close_wait list */ > > if (session->state == PIPEX_STATE_CLOSE_WAIT) > > diff --git sys/net/pipex_local.h sys/net/pipex_local.h > > index cf02c8e..ad3c3d3 100644 > > --- sys/net/pipex_local.h > > +++ sys/net/pipex_local.h > > @@ -369,7 +369,8 @@ extern struct pipex_hash_head pipex_id_hashtable[]; > > void pipex_iface_start (struct pipex_iface_context *); > > void pipex_iface_stop (struct pipex_iface_context *); > > int pipex_add_session (struct pipex_session_req *, > > struct pipex_iface_context *); > > -int pipex_close_session (struct pipex_session_close_req > > *); > > +int pipex_close_session (struct pipex_session_close_req > > *, > > + struct pipex_iface_context *); > > int pipex_config_session (struct > > pipex_session_config_req *); > > int pipex_get_stat (struct pipex_session_stat_req *); > > int pipex_get_closed (struct pipex_session_list_req *); > > ---- cut end > > > > This diff add ownership checks to the rest pipex_ioctl() commands. A few > > words about pppx_get_closed(): since in-kernel timeout feature was > > disabled for pppx(4) related pipex_sessions, closed pipex_sessions can't > > exist in system, so this function is dummy. I have an idea how to > > reenable this disabled timeout, but some reafactoring requited, and fair > > pipex_ioctl(PIPEXGCLOSED) call will be restored. > > > > ---- cut begin > > diff --git sys/net/if_pppx.c sys/net/if_pppx.c > > index 37a6af0..6c4977d 100644 > > --- sys/net/if_pppx.c > > +++ sys/net/if_pppx.c > > @@ -175,6 +175,12 @@ int pppx_add_session(struct pppx_dev *, > > struct pipex_session_req *); > > int pppx_del_session(struct pppx_dev *, > > struct pipex_session_close_req *); > > +int pppx_config_session(struct pppx_dev *, > > + struct pipex_session_config_req *); > > +int pppx_get_stat(struct pppx_dev *, > > + struct pipex_session_stat_req *); > > +int pppx_get_closed(struct pppx_dev *, > > + struct pipex_session_list_req *); > > int pppx_set_session_descr(struct pppx_dev *, > > struct pipex_session_descr_req *); > > > > @@ -451,16 +457,18 @@ pppxioctl(dev_t dev, u_long cmd, caddr_t addr, int > > flags, struct proc *p) > > break; > > > > case PIPEXCSESSION: > > - error = pipex_config_session( > > + error = pppx_config_session(pxd, > > (struct pipex_session_config_req *)addr); > > break; > > > > case PIPEXGSTAT: > > - error = pipex_get_stat((struct pipex_session_stat_req *)addr); > > + error = pppx_get_stat(pxd, > > + (struct pipex_session_stat_req *)addr); > > break; > > > > case PIPEXGCLOSED: > > - error = pipex_get_closed((struct pipex_session_list_req *)addr); > > + error = pppx_get_closed(pxd, > > + (struct pipex_session_list_req *)addr); > > break; > > > > case PIPEXSIFDESCR: > > @@ -947,6 +955,40 @@ pppx_del_session(struct pppx_dev *pxd, struct > > pipex_session_close_req *req) > > return (0); > > } > > > > +int > > +pppx_config_session(struct pppx_dev *pxd, > > + struct pipex_session_config_req *req) > > +{ > > + struct pppx_if *pxi; > > + > > + pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol); > > + if (pxi == NULL) > > + return (EINVAL); > > + > > + return pipex_config_session(req, &pxi->pxi_ifcontext); > > +} > > + > > +int > > +pppx_get_stat(struct pppx_dev *pxd, struct pipex_session_stat_req *req) > > +{ > > + struct pppx_if *pxi; > > + > > + pxi = pppx_if_find(pxd, req->psr_session_id, req->psr_protocol); > > + if (pxi == NULL) > > + return (EINVAL); > > + > > + return pipex_get_stat(req, &pxi->pxi_ifcontext); > > +} > > + > > +int > > +pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req) > > +{ > > + /* XXX: Only opened sessions exist for pppx(4) */ > > + memset(req, 0, sizeof(*req)); > > + > > + return 0; > > +} > > + > > int > > pppx_set_session_descr(struct pppx_dev *pxd, > > struct pipex_session_descr_req *req) > > diff --git sys/net/pipex.c sys/net/pipex.c > > index 22edce3..219e18d 100644 > > --- sys/net/pipex.c > > +++ sys/net/pipex.c > > @@ -235,15 +235,17 @@ pipex_ioctl(struct pipex_iface_context *pipex_iface, > > u_long cmd, caddr_t data) > > > > case PIPEXCSESSION: > > ret = pipex_config_session( > > - (struct pipex_session_config_req *)data); > > + (struct pipex_session_config_req *)data, pipex_iface); > > break; > > > > case PIPEXGSTAT: > > - ret = pipex_get_stat((struct pipex_session_stat_req *)data); > > + ret = pipex_get_stat((struct pipex_session_stat_req *)data, > > + pipex_iface); > > break; > > > > case PIPEXGCLOSED: > > - ret = pipex_get_closed((struct pipex_session_list_req *)data); > > + ret = pipex_get_closed((struct pipex_session_list_req *)data, > > + pipex_iface); > > break; > > > > default: > > @@ -514,7 +516,8 @@ pipex_close_session(struct pipex_session_close_req *req, > > } > > > > Static int > > -pipex_config_session(struct pipex_session_config_req *req) > > +pipex_config_session(struct pipex_session_config_req *req, > > + struct pipex_iface_context *iface) > > { > > struct pipex_session *session; > > > > @@ -523,36 +526,44 @@ pipex_config_session(struct pipex_session_config_req > > *req) > > req->pcr_session_id); > > if (session == NULL) > > return (EINVAL); > > + if (session->pipex_iface != iface) > > + return (EINVAL); > > session->ip_forward = req->pcr_ip_forward; > > > > return (0); > > } > > > > Static int > > -pipex_get_stat(struct pipex_session_stat_req *req) > > +pipex_get_stat(struct pipex_session_stat_req *req, > > + struct pipex_iface_context *iface) > > { > > struct pipex_session *session; > > > > NET_ASSERT_LOCKED(); > > session = pipex_lookup_by_session_id(req->psr_protocol, > > req->psr_session_id); > > - if (session == NULL) { > > + if (session == NULL) > > + return (EINVAL); > > + if (session->pipex_iface != iface) > > return (EINVAL); > > - } > > req->psr_stat = session->stat; > > > > return (0); > > } > > > > Static int > > -pipex_get_closed(struct pipex_session_list_req *req) > > +pipex_get_closed(struct pipex_session_list_req *req, > > + struct pipex_iface_context *iface) > > { > > - struct pipex_session *session; > > + struct pipex_session *session, *session_next; > > > > NET_ASSERT_LOCKED(); > > bzero(req, sizeof(*req)); > > - while (!LIST_EMPTY(&pipex_close_wait_list)) { > > - session = LIST_FIRST(&pipex_close_wait_list); > > + for (session = LIST_FIRST(&pipex_close_wait_list); > > + session; session = session_next) { > > + session_next = LIST_NEXT(session, state_list); > > + if (session->pipex_iface != iface) > > + continue; > > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id; > > LIST_REMOVE(session, state_list); > > session->state = PIPEX_STATE_CLOSE_WAIT2; > > diff --git sys/net/pipex_local.h sys/net/pipex_local.h > > index ad3c3d3..c124eb9 100644 > > --- sys/net/pipex_local.h > > +++ sys/net/pipex_local.h > > @@ -371,9 +371,12 @@ void pipex_iface_stop (struct > > pipex_iface_context *); > > int pipex_add_session (struct pipex_session_req *, > > struct pipex_iface_context *); > > int pipex_close_session (struct pipex_session_close_req > > *, > > struct pipex_iface_context *); > > -int pipex_config_session (struct > > pipex_session_config_req *); > > -int pipex_get_stat (struct pipex_session_stat_req *); > > -int pipex_get_closed (struct pipex_session_list_req *); > > +int pipex_config_session (struct > > pipex_session_config_req *, > > + struct pipex_iface_context *); > > +int pipex_get_stat (struct pipex_session_stat_req *, > > + struct pipex_iface_context *); > > +int pipex_get_closed (struct pipex_session_list_req *, > > + struct pipex_iface_context *); > > int pipex_destroy_session (struct pipex_session *); > > struct pipex_session *pipex_lookup_by_ip_address (struct in_addr); > > struct pipex_session *pipex_lookup_by_session_id (int, int); > > ---- cut end > >
