Re: fix sasyncd(8) race condition causing a segfault when getting data from the kernel
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 - 1.15
> +++ monitor.c 4 Sep 2012 13:06:37 -
> @@ -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_tv;
>
> 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");
> -
fix sasyncd(8) race condition causing a segfault when getting data from the kernel
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 - 1.15
+++ monitor.c 4 Sep 2012 13:06:37 -
@@ -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_tv;
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:
