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?
> ---- 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
