Skip to content

Commit

Permalink
ping: Lower max allowed -s value back to 127992
Browse files Browse the repository at this point in the history 
This effectively reverts a647e2c.

The implementation was wrong (signed integer overflow when -s >
INT_MAX * 0.58.

a647e2c commit increased value max allowed -s value from 127992 to
INT_MAX (2147483647). To have space for this range datalen was changed
from signed int to size_t, but it forgot to change other int variables
which also work with this data (hold, packlen) to be able to store
values > INT_MAX*2/3.

But these variables are used as setsockopt() parameter and passing
size_t to it is not a good idea: do_ip_setsockopt() and
do_ipv6_setsockopt() in kernel net/ipv4/ip_sockglue.c
net/ipv6/ipv6_sockglue.c does:

if (optlen >= sizeof(int)) {
    if (copy_from_sockptr(&val, optval, sizeof(val)))
	return -EFAULT;
}

If bigger size than sizeof(int) passed, kernel will take a first
sizeof(int) bytes from the value and use that. On 64bit the sizeof(int)
== 32 and sizeof(size_t) == 64. Therefore kernel takes only the half of
the value, which is working on little-endian machines. But on big-endian
we will take the upper half of the integer and wrongly interpret the
value as 0.

Proof that the implementation never worked as expected is that reverting
fixes signed integer overflow:

    $ export CC="clang"
    $ export CFLAGS="-O0 -g -fsanitize=address,undefined"
    $ export LDFLAGS="-O0 -g -fsanitize=address,undefined"
    $ ./configure && make
    $ ./builddir/ping/ping -s 2147483647 ::1
    ../ping/ping6_common.c:317:7: runtime error: signed integer overflow: -2147483641 + -1174404560 cannot be represented in type 'int'
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../ping/ping6_common.c:317:7
    ./builddir/ping/ping: WARNING: probably, rcvbuf is not enough to hold preload
    PING ::1 (::1) 2147483647 data bytes

    $ ./builddir/ping/ping -s 2147483647 127.0.0.1
    ../ping/ping.c:997:7: runtime error: signed integer overflow: -2147483641 + -1090518520 cannot be represented in type 'int'
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../ping/ping.c:997:7
    ./builddir/ping/ping: WARNING: probably, rcvbuf is not enough to hold preload
    PING 127.0.0.1 (127.0.0.1) 2147483647(2147483675) bytes of data.

Fixes: iputils#542
Reported-by: Vladimir Ryabokon <[email protected]>
Reviewed-by: Cyril Hrubis <[email protected]>
Reviewed-by: Benjamin Poirier <[email protected]>
Signed-off-by: Petr Vorel <[email protected]>
  • Loading branch information
@pevik
pevik committed Sep 2, 2024
1 parent aaa3dc3 commit db6257a
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 32 deletions.
5 changes: 3 additions & 2 deletions doc/ping.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,9 @@ xml:id="man.ping">
<listitem>
<para>Specifies the number of data bytes to be sent. The
default is 56, which translates into 64 ICMP data bytes
when combined with the 8 bytes of ICMP header
data.</para>
when combined with the 8 bytes of ICMP header data.
Maximum allowed value is 127992, but though most systems
limit this to a smaller, system-dependent number.</para>
</listitem>
</varlistentry>
<varlistentry>
Expand Down
24 changes: 8 additions & 16 deletions ping/ping.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,6 @@ main(int argc, char **argv)
socket_st sock4 = { .fd = -1 };
socket_st sock6 = { .fd = -1 };
char *target;
char *outpack_fill = NULL;
static struct ping_rts rts = {
.interval = 1000,
.preload = 1,
Expand Down Expand Up @@ -515,10 +514,7 @@ main(int argc, char **argv)
break;
case 'p':
rts.opt_pingfilled = 1;
free(outpack_fill);
outpack_fill = strdup(optarg);
if (!outpack_fill)
error(2, errno, _("memory allocation failed"));
fill(&rts, optarg, rts.outpack, sizeof(rts.outpack));
break;
case 'q':
rts.opt_quiet = 1;
Expand All @@ -531,7 +527,7 @@ main(int argc, char **argv)
rts.opt_so_dontroute = 1;
break;
case 's':
rts.datalen = strtol_or_err(optarg, _("invalid argument"), 0, INT_MAX);
rts.datalen = strtol_or_err(optarg, _("invalid argument"), 0, MAXPACKET - 8);
break;
case 'S':
rts.sndbuf = strtol_or_err(optarg, _("invalid argument"), 1, INT_MAX);
Expand Down Expand Up @@ -578,14 +574,6 @@ main(int argc, char **argv)

target = argv[argc - 1];

rts.outpack = malloc(rts.datalen + 28);
if (!rts.outpack)
error(2, errno, _("memory allocation failed"));
if (outpack_fill) {
fill(&rts, outpack_fill, rts.outpack, rts.datalen);
free(outpack_fill);
}

/* Create sockets */
enable_capability_raw();

Expand Down Expand Up @@ -695,7 +683,6 @@ main(int argc, char **argv)
}

freeaddrinfo(result);
free(rts.outpack);

return ret_val;
}
Expand Down Expand Up @@ -1017,6 +1004,11 @@ int ping4_run(struct ping_rts *rts, int argc, char **argv, struct addrinfo *ai,
error(2, errno, _("cannot set unicast time-to-live"));
}


if (rts->datalen > 0xFFFF - 8 - rts->optlen - 20)
error(2, 0, _("packet size %d is too large. Maximum is %d"),
rts->datalen, 0xFFFF - 8 - 20 - rts->optlen);

if (rts->datalen >= (int)sizeof(struct timeval)) /* can we time transfer */
rts->timing = 1;
packlen = rts->datalen + MAXIPLEN + MAXICMPLEN;
Expand All @@ -1026,7 +1018,7 @@ int ping4_run(struct ping_rts *rts, int argc, char **argv, struct addrinfo *ai,
printf(_("PING %s (%s) "), rts->hostname, inet_ntoa(rts->whereto.sin_addr));
if (rts->device || rts->opt_strictsource)
printf(_("from %s %s: "), inet_ntoa(rts->source.sin_addr), rts->device ? rts->device : "");
printf(_("%zu(%zu) bytes of data.\n"), rts->datalen, rts->datalen + 8 + rts->optlen + 20);
printf(_("%d(%d) bytes of data.\n"), rts->datalen, rts->datalen + 8 + rts->optlen + 20);

setup(rts, sock);
if (rts->opt_connect_sk &&
Expand Down
8 changes: 5 additions & 3 deletions ping/ping.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ typedef uint32_t bitmap_t;
# error Please MAX_DUP_CHK and/or BITMAP_SHIFT
#endif

#define MAXPACKET 128000 /* max packet size */

struct rcvd_table {
bitmap_t bitmap[MAX_DUP_CHK / (sizeof(bitmap_t) * 8)];
};
Expand Down Expand Up @@ -152,11 +154,11 @@ struct ping_ni {
/*ping runtime state */
struct ping_rts {
unsigned int mark;
unsigned char *outpack;
unsigned char outpack[MAXPACKET];

struct rcvd_table rcvd_tbl;

size_t datalen;
int datalen;
char *hostname;
uid_t uid;
int ident; /* process id to identify our packets */
Expand Down Expand Up @@ -413,7 +415,7 @@ extern int gather_statistics(struct ping_rts *rts, uint8_t *icmph, int icmplen,
void (*pr_reply)(uint8_t *ptr, int cc), int multicast,
int wrong_source);
extern void print_timestamp(struct ping_rts *rts);
void fill(struct ping_rts *rts, char *patp, unsigned char *packet, size_t packet_size);
void fill(struct ping_rts *rts, char *patp, unsigned char *packet, unsigned packet_size);

/* IPv6 */

Expand Down
2 changes: 1 addition & 1 deletion ping/ping6_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ int ping6_run(struct ping_rts *rts, int argc, char **argv, struct addrinfo *ai,
printf(_("from %s %s: "), pr_addr(rts, &rts->source6, sizeof rts->source6), rts->device ? rts->device : "");
rts->opt_numeric = saved_opt_numeric;
}
printf(_("%zu data bytes\n"), rts->datalen);
printf(_("%d data bytes\n"), rts->datalen);

setup(rts, sock);

Expand Down
18 changes: 8 additions & 10 deletions ping/ping_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ void drop_capabilities(void)
/* Fills all the outpack, excluding ICMP header, but _including_
* timestamp area with supplied pattern.
*/
void fill(struct ping_rts *rts, char *patp, unsigned char *packet, size_t packet_size)
void fill(struct ping_rts *rts, char *patp, unsigned char *packet, unsigned packet_size)
{
int ii, jj;
unsigned int pat[16];
Expand All @@ -224,10 +224,8 @@ void fill(struct ping_rts *rts, char *patp, unsigned char *packet, size_t packet
&pat[12], &pat[13], &pat[14], &pat[15]);

if (ii > 0) {
size_t kk;
size_t max = packet_size < (size_t)ii + 8 ? 0 : packet_size - (size_t)ii + 8;

for (kk = 0; kk <= max; kk += ii)
unsigned kk;
for (kk = 0; kk <= packet_size - (8 + ii); kk += ii)
for (jj = 0; jj < ii; ++jj)
bp[jj + kk] = pat[jj];
}
Expand Down Expand Up @@ -541,7 +539,7 @@ void setup(struct ping_rts *rts, socket_st *sock)
rts->opt_flood_poll = 1;

if (!rts->opt_pingfilled) {
size_t i;
int i;
unsigned char *p = rts->outpack + 8;

/* Do not forget about case of small datalen, fill timestamp area too! */
Expand Down Expand Up @@ -804,7 +802,7 @@ int gather_statistics(struct ping_rts *rts, uint8_t *icmph, int icmplen,
else
write_stdout("\bC", 2);
} else {
size_t i;
int i;
uint8_t *cp, *dp;

print_timestamp(rts);
Expand All @@ -819,7 +817,7 @@ int gather_statistics(struct ping_rts *rts, uint8_t *icmph, int icmplen,
if (hops >= 0)
printf(_(" ttl=%d"), hops);

if ((size_t)cc < rts->datalen + 8) {
if (cc < rts->datalen + 8) {
printf(_(" (truncated)\n"));
return 1;
}
Expand Down Expand Up @@ -851,12 +849,12 @@ int gather_statistics(struct ping_rts *rts, uint8_t *icmph, int icmplen,
dp = &rts->outpack[8 + sizeof(struct timeval)];
for (i = sizeof(struct timeval); i < rts->datalen; ++i, ++cp, ++dp) {
if (*cp != *dp) {
printf(_("\nwrong data byte #%zu should be 0x%x but was 0x%x"),
printf(_("\nwrong data byte #%d should be 0x%x but was 0x%x"),
i, *dp, *cp);
cp = (unsigned char *)ptr + sizeof(struct timeval);
for (i = sizeof(struct timeval); i < rts->datalen; ++i, ++cp) {
if ((i % 32) == sizeof(struct timeval))
printf("\n#%zu\t", i);
printf("\n#%d\t", i);
printf("%x ", *cp);
}
break;
Expand Down

0 comments on commit db6257a

Please sign in to comment.