Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
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_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_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
Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
On Sat, Apr 04, 2020 at 06:52:49PM +0200, Martin Pieuchot wrote:
> On 02/04/20(Thu) 13:44, Vitaliy Makkoveev wrote:
> > 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.
>
> The ownership looks good to me.
>
> I'd suggest we return an error for PIPEXGCLOSED documenting why this
> ioctl(2) is no longer supported. That could have been part of the
> previous fix that disabled the timeout. No need for a separate
> function.
We will restore timeout later. And after some additional steps
PIPEXGCLOSED will return fair list of PIPEX_STATE_CLOSE_WAIT stated
sessions associated with pppx_dev.
> See there's a conceptual problem with pipex_get_closed(). This function
> never returns an error, so when npppd(8) will issue the ioctl, even if
> an error occurs it will proceed with empty data.
npppd(8) does this check. See usr.sbin/npppd/npppd/npppd.c:1327
>
> Would it make sense to have two separated diffs?
I split it because only PEXDSESSION will crash. But since these diff add
the same checks they can be combined to one.
>
> > 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 @@ intpppx_add_session(struct pppx_dev *,
> > struct pipex_session_req *);
> > intpppx_del_session(struct pppx_dev *,
> > struct pipex_session_close_req *);
> > +intpppx_config_session(struct pppx_dev *,
> > + struct pipex_session_config_req *);
> > +intpppx_get_stat(struct pppx_dev *,
> > + struct pipex_session_stat_req *);
> > +intpppx_get_closed(struct pppx_dev *,
> > + struct pipex_session_list_req *);
> > intpppx_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_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_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);
> >
Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
On 02/04/20(Thu) 13:44, Vitaliy Makkoveev wrote:
> 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.
The ownership looks good to me.
I'd suggest we return an error for PIPEXGCLOSED documenting why this
ioctl(2) is no longer supported. That could have been part of the
previous fix that disabled the timeout. No need for a separate
function.
See there's a conceptual problem with pipex_get_closed(). This function
never returns an error, so when npppd(8) will issue the ioctl, even if
an error occurs it will proceed with empty data.
Would it make sense to have two separated diffs?
> 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_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_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;
>
> @@
Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
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=15854078841=1=2 and
finish cleanups and deduplication.
>
> > cut begin
> >
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> >
> > #include
> > #include
> > #include
> > #include
> >
> > #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(, 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", _ip_srcaddr);
> > inet_aton("10.0.0.2", _ip_address);
> > inet_aton("255.255.255.255", _ip_netmask);
> >
> > if(ioctl(fd_pppx, PIPEXASESSION, )<0){
> > err(1, "ioctl(fd_pppx, PIPEXASESSION)");
> > }
> >
> > memset(, 0, sizeof(reqd));
> > reqd.pcr_protocol=PIPEX_PROTO_L2TP;
> >
> > if(ioctl(fd_pppac, PIPEXDSESSION, )<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);
> >
Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
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
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> #include
> #include
> #include
> #include
>
> #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(, 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", _ip_srcaddr);
> inet_aton("10.0.0.2", _ip_address);
> inet_aton("255.255.255.255", _ip_netmask);
>
> if(ioctl(fd_pppx, PIPEXASESSION, )<0){
> err(1, "ioctl(fd_pppx, PIPEXASESSION)");
> }
>
> memset(, 0, sizeof(reqd));
> reqd.pcr_protocol=PIPEX_PROTO_L2TP;
>
> if(ioctl(fd_pppac, PIPEXDSESSION, )<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
> +++
Re: Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
Sorry, screenshot was from pached kernel. Screenshot from clean kernel included.
Fix pipex(4) pipex_ioctl() access to not owned sessions (kernel crash too)
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.
cut begin
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#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(, 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", _ip_srcaddr);
inet_aton("10.0.0.2", _ip_address);
inet_aton("255.255.255.255", _ip_netmask);
if(ioctl(fd_pppx, PIPEXASESSION, )<0){
err(1, "ioctl(fd_pppx, PIPEXASESSION)");
}
memset(, 0, sizeof(reqd));
reqd.pcr_protocol=PIPEX_PROTO_L2TP;
if(ioctl(fd_pppac, PIPEXDSESSION, )<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 @@ intpppx_add_session(struct pppx_dev *,
struct pipex_session_req *);
intpppx_del_session(struct pppx_dev *,
struct pipex_session_close_req *);
+intpppx_config_session(struct pppx_dev *,
+ struct pipex_session_config_req *);
+intpppx_get_stat(struct pppx_dev *,
+
