On Fri, Aug 30, 2019 at 04:44:56PM +0200, Alexander Bluhm wrote:
> Hi,
>
> The algorithm in route(8) and arp(6) is still not correct. While
> the values written to the kernel are fine, the bytes for padding
> are taken from memory after the sockaddr structs.
>
> In route(8) the union of sockaddr can be made larger so that the
> padding is taken from there.
>
> In arp(8) we know the size of the struct. Copy only the struct and
> advance over the padding. The memory has been zeroed before.
>
> ndp(8) can take all the fixes from arp(8).
>
> ok?
>
> bluhm
>
> Index: sbin/route/route.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sbin/route/route.c,v
> retrieving revision 1.232
> diff -u -p -r1.232 route.c
> --- sbin/route/route.c 29 Aug 2019 22:42:16 -0000 1.232
> +++ sbin/route/route.c 30 Aug 2019 13:24:31 -0000
> @@ -137,10 +137,6 @@ usage(char *cp)
> exit(1);
> }
>
> -#define ROUNDUP(a) \
> - ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
> -#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
> -
> int
> main(int argc, char **argv)
> {
> Index: sbin/route/show.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sbin/route/show.h,v
> retrieving revision 1.14
> diff -u -p -r1.14 show.h
> --- sbin/route/show.h 1 May 2018 18:13:21 -0000 1.14
> +++ sbin/route/show.h 30 Aug 2019 13:22:57 -0000
> @@ -19,6 +19,18 @@
> #ifndef __SHOW_H__
> #define __SHOW_H__
>
> +#define ROUNDUP(a) \
> + ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
> +#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
> +#define MAXIMUM(a, b) ((a) > (b) ? (a) : (b))
> +#define SAMAXSIZE \
> + MAXIMUM(sizeof(struct sockaddr), \
> + MAXIMUM(sizeof(struct sockaddr_in), \
> + MAXIMUM(sizeof(struct sockaddr_in6), \
> + MAXIMUM(sizeof(struct sockaddr_dl), \
> + MAXIMUM(sizeof(struct sockaddr_rtlabel), \
> + sizeof(struct sockaddr_mpls))))))
> +
> union sockunion {
> struct sockaddr sa;
> struct sockaddr_in sin;
> @@ -26,6 +38,7 @@ union sockunion {
> struct sockaddr_dl sdl;
> struct sockaddr_rtlabel rtlabel;
> struct sockaddr_mpls smpls;
> + char padding[ROUNDUP(SAMAXSIZE)];
Just throw a struct sockaddr_storage in that union. It will make sure
there is enough space for everything and then you can skip the MAXIMUM
dance you do now.
> };
>
> void get_rtaddrs(int, struct sockaddr *, struct sockaddr **);
> Index: usr.sbin/arp/arp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/arp/arp.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 arp.c
> --- usr.sbin/arp/arp.c 29 Aug 2019 19:11:15 -0000 1.85
> +++ usr.sbin/arp/arp.c 30 Aug 2019 13:57:15 -0000
> @@ -675,9 +675,8 @@ rtmsg(int cmd)
>
> #define NEXTADDR(w, s)
> \
> if (rtm->rtm_addrs & (w)) { \
> - l = ROUNDUP(((struct sockaddr *)&(s))->sa_len); \
> - memcpy(cp, &(s), l); \
> - cp += l; \
> + memcpy(cp, &(s), sizeof(s)); \
> + ADVANCE(cp, (struct sockaddr *)&(s)); \
> }
>
> NEXTADDR(RTA_DST, sin_m);
> Index: usr.sbin/ndp/ndp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/ndp/ndp.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 ndp.c
> --- usr.sbin/ndp/ndp.c 27 Aug 2019 20:42:40 -0000 1.97
> +++ usr.sbin/ndp/ndp.c 30 Aug 2019 14:08:52 -0000
> @@ -108,6 +108,7 @@
> /* packing rule for routing socket */
> #define ROUNDUP(a) \
> ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long))
> +#define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
>
> static pid_t pid;
> static int nflag;
> @@ -323,7 +324,7 @@ parse_host(const char *host, struct in6_
> struct sockaddr_in6 so_mask = {sizeof(so_mask), AF_INET6 };
> struct sockaddr_in6 blank_sin = {sizeof(blank_sin), AF_INET6 }, sin_m;
> struct sockaddr_dl blank_sdl = {sizeof(blank_sdl), AF_LINK }, sdl_m;
> -struct sockaddr_dl ifp_m = { sizeof(&ifp_m), AF_LINK };
> +struct sockaddr_dl ifp_m = { sizeof(ifp_m), AF_LINK };
> time_t expire_time;
> int flags, found_entry;
> struct {
> @@ -766,9 +767,12 @@ rtmsg(int cmd)
> case RTM_GET:
> rtm->rtm_addrs |= (RTA_DST | RTA_IFP);
> }
> -#define NEXTADDR(w, s) \
> - if (rtm->rtm_addrs & (w)) { \
> - bcopy((char *)&s, cp, sizeof(s)); cp += ROUNDUP(sizeof(s));}
> +
> +#define NEXTADDR(w, s)
> \
> + if (rtm->rtm_addrs & (w)) { \
> + memcpy(cp, &(s), sizeof(s)); \
> + ADVANCE(cp, (struct sockaddr *)&(s)); \
> + }
>
> NEXTADDR(RTA_DST, sin_m);
> NEXTADDR(RTA_GATEWAY, sdl_m);
> @@ -825,7 +829,7 @@ rtget(struct sockaddr_in6 **sinp, struct
> default:
> break;
> }
> - cp += ROUNDUP(sa->sa_len);
> + ADVANCE(cp, sa);
> }
> }
> }
>
Rest I agree with.
--
:wq Claudio
