I've reviewed this diff with Patrick and I believe it is correct, can
someone else have a look ?
On Tue, Sep 04, 2012 at 04:46:08PM +0200, Patrick Wildt wrote:
> When sasyncd(8) dumps data from the kernel, it does a sysctl() for the size to
> alloc,
> and a second for the actual data. In between, new data can be added/deleted.
> Therefore
> the kernel might return less data, than actually told. The case that you get
> less/more data
> isn't handled properly.
>
> In case of less data, it might happen that the child is working on
> uninitialized data.
> This might lead to a segmentation fault.
> In case of more data, the sasyncd(8) parent process handles it as he didn't
> get any data.
>
> This diff rewrites that part, so that we try thrice to get the SAs and SPDs.
> If getting the SAs
> breaks thrice, we tell the child process, that there's no data. Same for the
> SPDs.
>
> The case of less data is now handled properly. The case of more data is either
> caught
> by the for loop, or allocing twice the size of the data we should originally
> get.
> That might be helpful on bigger vpn setups, when the loop still doesn't catch
> the race.
>
> Tested with 7080 SAs. (For real.)
>
> Index: monitor.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/sasyncd/monitor.c,v
> retrieving revision 1.15
> diff -u -r1.15 monitor.c
> --- monitor.c 2 Apr 2012 18:56:15 -0000 1.15
> +++ monitor.c 4 Sep 2012 13:06:37 -0000
> @@ -342,7 +342,7 @@
> {
> u_int8_t *sadb_buf = 0, *spd_buf = 0;
> size_t sadb_buflen = 0, spd_buflen = 0, sz;
> - int mib[5];
> + int i, mib[5];
> u_int32_t v;
>
> mib[0] = CTL_NET;
> @@ -352,59 +352,81 @@
> mib[4] = 0; /* Unspec SA type */
>
> /* First, fetch SADB data */
> - if (sysctl(mib, sizeof mib / sizeof mib[0], NULL, &sz, NULL, 0) == -1
> - || sz == 0) {
> - sadb_buflen = 0;
> - goto try_spd;
> - }
> -
> - sadb_buflen = sz;
> - if ((sadb_buf = malloc(sadb_buflen)) == NULL) {
> - log_err("m_priv_pfkey_snap: malloc");
> - sadb_buflen = 0;
> - goto out;
> - }
> -
> - if (sysctl(mib, sizeof mib / sizeof mib[0], sadb_buf, &sz, NULL, 0)
> - == -1) {
> - log_err("m_priv_pfkey_snap: sysctl");
> - sadb_buflen = 0;
> - goto out;
> + for (i = 0; i < 3; i++) {
> + if (sysctl(mib, sizeof mib / sizeof mib[0], NULL, &sz, NULL, 0)
> + == -1)
> + break;
> +
> + if (!sz)
> + break;
> +
> + /* Try to catch newly added data */
> + sz *= 2;
> +
> + if ((sadb_buf = malloc(sz)) == NULL)
> + break;
> +
> + if (sysctl(mib, sizeof mib / sizeof mib[0], sadb_buf, &sz,
> NULL, 0)
> + == -1) {
> + free(sadb_buf);
> + sadb_buf = 0;
> + /*
> + * If new SAs were added meanwhile and the given buffer
> is
> + * too small, retry.
> + */
> + if (errno == ENOMEM)
> + continue;
> + break;
> + }
> +
> + sadb_buflen = sz;
> + break;
> }
>
> /* Next, fetch SPD data */
> - try_spd:
> mib[3] = NET_KEY_SPD_DUMP;
>
> - if (sysctl(mib, sizeof mib / sizeof mib[0], NULL, &sz, NULL, 0) == -1
> - || sz == 0) {
> - spd_buflen = 0;
> - goto out;
> - }
> + for (i = 0; i < 3; i++) {
> + if (sysctl(mib, sizeof mib / sizeof mib[0], NULL, &sz, NULL, 0)
> + == -1)
> + break;
>
> - spd_buflen = sz;
> - if ((spd_buf = malloc(spd_buflen)) == NULL) {
> - log_err("m_priv_pfkey_snap: malloc");
> - spd_buflen = 0;
> - goto out;
> - }
> + if (!sz)
> + break;
> +
> + /* Try to catch newly added data */
> + sz *= 2;
> +
> + if ((spd_buf = malloc(sz)) == NULL)
> + break;
> +
> + if (sysctl(mib, sizeof mib / sizeof mib[0], spd_buf, &sz, NULL,
> 0)
> + == -1) {
> + free(spd_buf);
> + spd_buf = 0;
> + /*
> + * If new SPDs were added meanwhile and the given
> buffer is
> + * too small, retry.
> + */
> + if (errno == ENOMEM)
> + continue;
> + break;
> + }
>
> - if (sysctl(mib, sizeof mib / sizeof mib[0], spd_buf, &sz, NULL, 0)
> - == -1) {
> - log_err("m_priv_pfkey_snap: sysctl");
> - spd_buflen = 0;
> - goto out;
> + spd_buflen = sz;
> + break;
> }
>
> - out:
> /* Return SADB data */
> v = (u_int32_t)sadb_buflen;
> if (m_write(s, &v, sizeof v) == -1) {
> log_err("m_priv_pfkey_snap: write");
> goto cleanup;
> }
> - if (m_write(s, sadb_buf, sadb_buflen) == -1)
> + if (m_write(s, sadb_buf, sadb_buflen) == -1) {
> log_err("m_priv_pfkey_snap: write");
> + goto cleanup;
> + }
>
> /* Return SPD data */
> v = (u_int32_t)spd_buflen;
> @@ -412,13 +434,20 @@
> log_err("m_priv_pfkey_snap: write");
> goto cleanup;
> }
> - if (m_write(s, spd_buf, spd_buflen) == -1)
> + if (m_write(s, spd_buf, spd_buflen) == -1) {
> log_err("m_priv_pfkey_snap: write");
> - cleanup:
> - memset(sadb_buf, 0, sadb_buflen);
> - free(sadb_buf);
> - memset(spd_buf, 0, spd_buflen);
> - free(spd_buf);
> + goto cleanup;
> + }
> +
> +cleanup:
> + if (sadb_buf) {
> + memset(sadb_buf, 0, sadb_buflen);
> + free(sadb_buf);
> + }
> + if (spd_buf) {
> + memset(spd_buf, 0, spd_buflen);
> + free(spd_buf);
> + }
> }
>
> static int
