On Thu, Apr 02, 2020 at 01:44:50PM +0300, 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.
>
...
> 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.
One minor comment below:
> ---- 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);
I would use a LIST_FOREACH_SAFE here
> + 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
Apart from that OK claudio@
--
:wq Claudio
