Re: Use uiomove() in if_tun and if_pppx

2016-01-23 Thread Martin Natano
For what it's worth, your diff reads fine to me. It took me a while to
realize, that tlen in pppxwrite() and tun_dev_write() can't overflow
because of the range check beforehand, but on close look that code also
turns out to be correct.

cheers,
natano


On Fri, Jan 22, 2016 at 07:29:54PM +0100, Stefan Kempf wrote:
> The following diff prevents integer truncation of uio_resid
> by using ulmin() instead of min() and calls uiomove() instead
> of the legacy uiomove().
> 
> That's straightforward because the m_len mbuf field is unsigned.
> mlen can be turned to a size_t because it's set to MLEN or MCLBYTES,
> which is > 0.
> 
> I plan to commit this in a few days unless there are objections.
> 
> Index: if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 if_pppx.c
> --- if_pppx.c 14 Jan 2016 09:20:31 -  1.49
> +++ if_pppx.c 22 Jan 2016 18:23:04 -
> @@ -273,7 +273,8 @@ pppxread(dev_t dev, struct uio *uio, int
>   struct pppx_dev *pxd = pppx_dev2pxd(dev);
>   struct mbuf *m, *m0;
>   int error = 0;
> - int len, s;
> + int s;
> + size_t len;
>  
>   if (!pxd)
>   return (ENXIO);
> @@ -292,9 +293,9 @@ pppxread(dev_t dev, struct uio *uio, int
>   }
>  
>   while (m0 != NULL && uio->uio_resid > 0 && error == 0) {
> - len = min(uio->uio_resid, m0->m_len);
> + len = ulmin(uio->uio_resid, m0->m_len);
>   if (len != 0)
> - error = uiomovei(mtod(m0, caddr_t), len, uio);
> + error = uiomove(mtod(m0, caddr_t), len, uio);
>   m = m_free(m0);
>   m0 = m;
>   }
> @@ -313,8 +314,9 @@ pppxwrite(dev_t dev, struct uio *uio, in
>   uint32_t proto;
>   struct mbuf *top, **mp, *m;
>   struct niqueue *ifq;
> - int tlen, mlen;
> + int tlen;
>   int error = 0;
> + size_t mlen;
>  #if NBPFILTER > 0
>   int s;
>  #endif
> @@ -342,8 +344,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
>   mp = 
>  
>   while (error == 0 && uio->uio_resid > 0) {
> - m->m_len = min(mlen, uio->uio_resid);
> - error = uiomovei(mtod (m, caddr_t), m->m_len, uio);
> + m->m_len = ulmin(mlen, uio->uio_resid);
> + error = uiomove(mtod (m, caddr_t), m->m_len, uio);
>   *mp = m;
>   mp = >m_next;
>   if (error == 0 && uio->uio_resid > 0) {
> Index: if_tun.c
> ===
> RCS file: /cvs/src/sys/net/if_tun.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 if_tun.c
> --- if_tun.c  7 Jan 2016 05:31:17 -   1.165
> +++ if_tun.c  22 Jan 2016 18:23:05 -
> @@ -764,7 +764,8 @@ tun_dev_read(struct tun_softc *tp, struc
>   struct ifnet*ifp = >tun_if;
>   struct mbuf *m, *m0;
>   unsigned int ifidx;
> - int  error = 0, len, s;
> + int  error = 0, s;
> + size_t   len;
>  
>   if ((tp->tun_flags & TUN_READY) != TUN_READY)
>   return (EHOSTDOWN);
> @@ -825,9 +826,9 @@ tun_dev_read(struct tun_softc *tp, struc
>   }
>  
>   while (m0 != NULL && uio->uio_resid > 0 && error == 0) {
> - len = min(uio->uio_resid, m0->m_len);
> + len = ulmin(uio->uio_resid, m0->m_len);
>   if (len != 0)
> - error = uiomovei(mtod(m0, caddr_t), len, uio);
> + error = uiomove(mtod(m0, caddr_t), len, uio);
>   m = m_free(m0);
>   m0 = m;
>   }
> @@ -872,7 +873,8 @@ tun_dev_write(struct tun_softc *tp, stru
>   struct niqueue  *ifq;
>   u_int32_t   *th;
>   struct mbuf *top, **mp, *m;
> - int  error=0, tlen, mlen;
> + int error = 0, tlen;
> + size_t  mlen;
>  #if NBPFILTER > 0
>   int  s;
>  #endif
> @@ -911,8 +913,8 @@ tun_dev_write(struct tun_softc *tp, stru
>   m->m_data += ETHER_ALIGN;
>   }
>   while (error == 0 && uio->uio_resid > 0) {
> - m->m_len = min(mlen, uio->uio_resid);
> - error = uiomovei(mtod (m, caddr_t), m->m_len, uio);
> + m->m_len = ulmin(mlen, uio->uio_resid);
> + error = uiomove(mtod (m, caddr_t), m->m_len, uio);
>   *mp = m;
>   mp = >m_next;
>   if (error == 0 && uio->uio_resid > 0) {
> 



Use uiomove() in if_tun and if_pppx

2016-01-22 Thread Stefan Kempf
The following diff prevents integer truncation of uio_resid
by using ulmin() instead of min() and calls uiomove() instead
of the legacy uiomove().

That's straightforward because the m_len mbuf field is unsigned.
mlen can be turned to a size_t because it's set to MLEN or MCLBYTES,
which is > 0.

I plan to commit this in a few days unless there are objections.

Index: if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.49
diff -u -p -r1.49 if_pppx.c
--- if_pppx.c   14 Jan 2016 09:20:31 -  1.49
+++ if_pppx.c   22 Jan 2016 18:23:04 -
@@ -273,7 +273,8 @@ pppxread(dev_t dev, struct uio *uio, int
struct pppx_dev *pxd = pppx_dev2pxd(dev);
struct mbuf *m, *m0;
int error = 0;
-   int len, s;
+   int s;
+   size_t len;
 
if (!pxd)
return (ENXIO);
@@ -292,9 +293,9 @@ pppxread(dev_t dev, struct uio *uio, int
}
 
while (m0 != NULL && uio->uio_resid > 0 && error == 0) {
-   len = min(uio->uio_resid, m0->m_len);
+   len = ulmin(uio->uio_resid, m0->m_len);
if (len != 0)
-   error = uiomovei(mtod(m0, caddr_t), len, uio);
+   error = uiomove(mtod(m0, caddr_t), len, uio);
m = m_free(m0);
m0 = m;
}
@@ -313,8 +314,9 @@ pppxwrite(dev_t dev, struct uio *uio, in
uint32_t proto;
struct mbuf *top, **mp, *m;
struct niqueue *ifq;
-   int tlen, mlen;
+   int tlen;
int error = 0;
+   size_t mlen;
 #if NBPFILTER > 0
int s;
 #endif
@@ -342,8 +344,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
mp = 
 
while (error == 0 && uio->uio_resid > 0) {
-   m->m_len = min(mlen, uio->uio_resid);
-   error = uiomovei(mtod (m, caddr_t), m->m_len, uio);
+   m->m_len = ulmin(mlen, uio->uio_resid);
+   error = uiomove(mtod (m, caddr_t), m->m_len, uio);
*mp = m;
mp = >m_next;
if (error == 0 && uio->uio_resid > 0) {
Index: if_tun.c
===
RCS file: /cvs/src/sys/net/if_tun.c,v
retrieving revision 1.165
diff -u -p -r1.165 if_tun.c
--- if_tun.c7 Jan 2016 05:31:17 -   1.165
+++ if_tun.c22 Jan 2016 18:23:05 -
@@ -764,7 +764,8 @@ tun_dev_read(struct tun_softc *tp, struc
struct ifnet*ifp = >tun_if;
struct mbuf *m, *m0;
unsigned int ifidx;
-   int  error = 0, len, s;
+   int  error = 0, s;
+   size_t   len;
 
if ((tp->tun_flags & TUN_READY) != TUN_READY)
return (EHOSTDOWN);
@@ -825,9 +826,9 @@ tun_dev_read(struct tun_softc *tp, struc
}
 
while (m0 != NULL && uio->uio_resid > 0 && error == 0) {
-   len = min(uio->uio_resid, m0->m_len);
+   len = ulmin(uio->uio_resid, m0->m_len);
if (len != 0)
-   error = uiomovei(mtod(m0, caddr_t), len, uio);
+   error = uiomove(mtod(m0, caddr_t), len, uio);
m = m_free(m0);
m0 = m;
}
@@ -872,7 +873,8 @@ tun_dev_write(struct tun_softc *tp, stru
struct niqueue  *ifq;
u_int32_t   *th;
struct mbuf *top, **mp, *m;
-   int  error=0, tlen, mlen;
+   int error = 0, tlen;
+   size_t  mlen;
 #if NBPFILTER > 0
int  s;
 #endif
@@ -911,8 +913,8 @@ tun_dev_write(struct tun_softc *tp, stru
m->m_data += ETHER_ALIGN;
}
while (error == 0 && uio->uio_resid > 0) {
-   m->m_len = min(mlen, uio->uio_resid);
-   error = uiomovei(mtod (m, caddr_t), m->m_len, uio);
+   m->m_len = ulmin(mlen, uio->uio_resid);
+   error = uiomove(mtod (m, caddr_t), m->m_len, uio);
*mp = m;
mp = >m_next;
if (error == 0 && uio->uio_resid > 0) {