[4/4] Fix SXID_ERASE behavior in setuid programs (BZ #27471)

When parse_tunables tries to erase a tunable marked as SXID_ERASE for
setuid programs, it ends up setting the envvar string iterator
incorrectly, because of which it may parse the next tunable
incorrectly.  Given that currently the implementation allows malformed
and unrecognized tunables pass through, it may even allow SXID_ERASE
tunables to go through.

This change revamps the SXID_ERASE implementation so that:

- Only valid tunables are written back to the tunestr string, because
  of which children of SXID programs will only inherit a clean list of
  identified tunables that are not SXID_ERASE.

- Unrecognized tunables get scrubbed off from the environment and
  subsequently from the child environment.

- This has the side-effect that a tunable that is not identified by
  the setxid binary, will not be passed on to a non-setxid child even
  if the child could have identified that tunable.  This may break
  applications that expect this behaviour but expecting such tunables
  to cross the SXID boundary is wrong.
---
 elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
 elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
 2 files changed, 52 insertions(+), 30 deletions(-)

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
> setuid programs, it ends up setting the envvar string iterator
> incorrectly, because of which it may parse the next tunable
> incorrectly.  Given that currently the implementation allows malformed
> and unrecognized tunables pass through, it may even allow SXID_ERASE
> tunables to go through.

I have read through bug 27471, thanks for referencing the bug.
 
I'm worried about the fact that we have existing examples from DJ's
nsswitch changes with / changing, that there are real-world examples
where we actively run a child process in a potentially newer glibc
and stripping away tunables for the current hardware means the child
process needs a way to add it back, and doing that isn't entirely
clear e.g. sudo with hardcoded env.

This makes testing and producing tuning difficult or impossible
and I that means to me that we should be more conservative with what
we process.

> This change revamps the SXID_ERASE implementation so that:
> 
> - Only valid tunables are written back to the tunestr string, because
>   of which children of SXID programs will only inherit a clean list of
>   identified tunables that are not SXID_ERASE.

This prevents passing a tunable through a program to a child that runs
with a newer glibc and accepts different tunables.

> - Unrecognized tunables get scrubbed off from the environment and
>   subsequently from the child environment.

I think we need to be conservative here and allow unknown tunables
to be "passed through" to the child without modification.

> - This has the side-effect that a tunable that is not identified by
>   the setxid binary, will not be passed on to a non-setxid child even
>   if the child could have identified that tunable.  This may break
>   applications that expect this behaviour but expecting such tunables
>   to cross the SXID boundary is wrong.

Why is it wrong?

We can have instances where we chroot or unshare into another glibc
version and we may want the env vars to pass through because they are
related to hardware changes.

If I step back and look at the bigger picture I see the following:

(1) Tunables we have removed.
- Did we record these?
- Did we record their security setting?
- Are we avoiding reusing them by accident?
- Are we cleaning legacy tunables based on their security setting to
  keep the environment clean for a child running with an old glibc?

(2) Tunables we don't know about.
- The child process may need to consume these because it runs a newer glibc.
- We lack /var/glibc.tunables handling of these tunables, or the child may be a
  generic container, or generic chroot, maybe env vars are the only way to pass
  this information.

(3) Tunables we know about.
- These are the straight forward fix, we just fix the code.

The patch handles (1) and (2) and (3) but picks an easier to audit
and verify solution for (3) at the expense of (1) and (2). I don't know if
that is a good tradeoff.

With my developer hat on I expect the following:
- All old legacy tunables are handled as their old security settings.
- All new tunables are handled as their current security settings.
- Unknown tunables are left alone.

Have I justified my expectation?

> ---
>  elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
>  elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
>  2 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..1aedb9bd36 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring)
>      return;
>  
>    char *p = tunestr;
> +  size_t off = 0;
>  
>    while (true)
>      {
> @@ -184,7 +185,11 @@ parse_tunables (char *tunestr, char *valstring)
>        /* If we reach the end of the string before getting a valid name-value
>  	 pair, bail out.  */
>        if (p[len] == '\0')
> -	return;
> +	{
> +	  if (__libc_enable_secure)
> +	    tunestr[off] = '\0';
> +	  return;
> +	}
>  
>        /* We did not find a valid name-value pair before encountering the
>  	 colon.  */
> @@ -210,35 +215,28 @@ parse_tunables (char *tunestr, char *valstring)
>  
>  	  if (tunable_is_name (cur->name, name))
>  	    {
> -	      /* If we are in a secure context (AT_SECURE) then ignore the tunable
> -		 unless it is explicitly marked as secure.  Tunable values take
> -		 precedence over their envvar aliases.  */
> +	      /* If we are in a secure context (AT_SECURE) then ignore the
> +		 tunable unless it is explicitly marked as secure.  Tunable
> +		 values take precedence over their envvar aliases.  We write
> +		 the tunables that are not SXID_ERASE back to TUNESTR, thus
> +		 dropping all SXID_ERASE tunables and any invalid or
> +		 unrecognized tunables.  */
>  	      if (__libc_enable_secure)
>  		{
> -		  if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> +		  if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>  		    {
> -		      if (p[len] == '\0')
> -			{
> -			  /* Last tunable in the valstring.  Null-terminate and
> -			     return.  */
> -			  *name = '\0';
> -			  return;
> -			}
> -		      else
> -			{
> -			  /* Remove the current tunable from the string.  We do
> -			     this by overwriting the string starting from NAME
> -			     (which is where the current tunable begins) with
> -			     the remainder of the string.  We then have P point
> -			     to NAME so that we continue in the correct
> -			     position in the valstring.  */
> -			  char *q = &p[len + 1];
> -			  p = name;
> -			  while (*q != '\0')
> -			    *name++ = *q++;
> -			  name[0] = '\0';
> -			  len = 0;
> -			}
> +		      if (off > 0)
> +			tunestr[off++] = ':';
> +
> +		      const char *n = cur->name;
> +
> +		      while (*n != '\0')
> +			tunestr[off++] = *n++;
> +
> +		      tunestr[off++] = '=';
> +
> +		      for (size_t j = 0; j < len; j++)
> +			tunestr[off++] = value[j];
>  		    }
>  
>  		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> @@ -251,9 +249,7 @@ parse_tunables (char *tunestr, char *valstring)
>  	    }
>  	}
>  
> -      if (p[len] == '\0')
> -	return;
> -      else
> +      if (p[len] != '\0')
>  	p += len + 1;
>      }
>  }
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 3d523875b1..05619c9adc 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -45,11 +45,37 @@
>  const char *teststrings[] =
>  {
>    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> +  "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
> +  ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> +  "glibc.malloc.check=1:glibc.malloc.check=2",
> +  "not_valid.malloc.check=2",
> +  "glibc.not_valid.check=2",
>  };
>  
>  const char *resultstrings[] =
>  {
>    "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
>  };
>  
>  static int
>

On 4/7/21 1:17 AM, Carlos O'Donell wrote:
> On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
>> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
>> setuid programs, it ends up setting the envvar string iterator
>> incorrectly, because of which it may parse the next tunable
>> incorrectly.  Given that currently the implementation allows malformed
>> and unrecognized tunables pass through, it may even allow SXID_ERASE
>> tunables to go through.
> 
> I have read through bug 27471, thanks for referencing the bug.
>   
> I'm worried about the fact that we have existing examples from DJ's
> nsswitch changes with / changing, that there are real-world examples
> where we actively run a child process in a potentially newer glibc
> and stripping away tunables for the current hardware means the child
> process needs a way to add it back, and doing that isn't entirely
> clear e.g. sudo with hardcoded env.
> 
> This makes testing and producing tuning difficult or impossible
> and I that means to me that we should be more conservative with what
> we process.
> 
>> This change revamps the SXID_ERASE implementation so that:
>>
>> - Only valid tunables are written back to the tunestr string, because
>>    of which children of SXID programs will only inherit a clean list of
>>    identified tunables that are not SXID_ERASE.
> 
> This prevents passing a tunable through a program to a child that runs
> with a newer glibc and accepts different tunables.
> 
>> - Unrecognized tunables get scrubbed off from the environment and
>>    subsequently from the child environment.
> 
> I think we need to be conservative here and allow unknown tunables
> to be "passed through" to the child without modification.
> 
>> - This has the side-effect that a tunable that is not identified by
>>    the setxid binary, will not be passed on to a non-setxid child even
>>    if the child could have identified that tunable.  This may break
>>    applications that expect this behaviour but expecting such tunables
>>    to cross the SXID boundary is wrong.
> 
> Why is it wrong?

That comment was based on a previous consensus[1] where we agreed that 
we eventually port all SXID_IGNORE tunables into SXID_ERASE but I never 
actually got around to doing it.  However you seem to arguing the 
opposite case, i.e. making tunables SXID_IGNORE by default and porting 
SXID_ERASE over to it.

As I've mentioned in the bug report, I don't see miscategorization of 
tunables resulting in any *new* security issues; at worst they may 
provide additional control over processes across setxid boundaries that 
may otherwise not be possible.  In that sense, sxid categorization is a 
security hardening feature and the discussion is about whether the 
hardening is going too far.

> We can have instances where we chroot or unshare into another glibc
> version and we may want the env vars to pass through because they are
> related to hardware changes.
> 
> If I step back and look at the bigger picture I see the following:
> 
> (1) Tunables we have removed.
> - Did we record these?
> - Did we record their security setting?
> - Are we avoiding reusing them by accident?
> - Are we cleaning legacy tunables based on their security setting to
>    keep the environment clean for a child running with an old glibc?

There was one namespace renaming from glibc.tune to glibc.cpu, but it 
was aarch64-specific.  It defaults to SXID_ERASE, as do most tunables. 
Only tunables that were ported from environment variables, e.g. 
glibc.malloc.mmap_threshold are SXID_IGNORE.

> (2) Tunables we don't know about.
> - The child process may need to consume these because it runs a newer glibc.
> - We lack /var/glibc.tunables handling of these tunables, or the child may be a
>    generic container, or generic chroot, maybe env vars are the only way to pass
>    this information.
> 
> (3) Tunables we know about.
> - These are the straight forward fix, we just fix the code.
> 
> The patch handles (1) and (2) and (3) but picks an easier to audit
> and verify solution for (3) at the expense of (1) and (2). I don't know if
> that is a good tradeoff.
> 
> With my developer hat on I expect the following:
> - All old legacy tunables are handled as their old security settings.
> - All new tunables are handled as their current security settings.
> - Unknown tunables are left alone.
> 
> Have I justified my expectation?

To summarize my understanding, you argue that there may be a legitimate 
need to be able to control environment of processes across the sxid 
boundary.  While I agree that there ought to be ways to control 
processes across sxid boundaries I don't think it should be as trivial 
as setting the program environment of an sxid program.

Maybe the use case you're referring to is better served by systemwide 
and userwide tunables.  For example, if I had to control the environment 
of httpd, it should be through $HTTPD_HOME/.glibc-tunables or 
/etc/glibc-tunables and not through $SIDDHESH_HOME/.glibc-tunables or 
the environment of user siddhesh.  This may not work for legacy tunables 
if the older environment doesn't support systemwide or userwide tunables 
but that shouldn't cause problems in practice.

Siddhesh

[1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html

On 4/8/21 12:38 AM, Siddhesh Poyarekar wrote:
> On 4/7/21 1:17 AM, Carlos O'Donell wrote:
>> On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
>>> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
>>> setuid programs, it ends up setting the envvar string iterator
>>> incorrectly, because of which it may parse the next tunable
>>> incorrectly.  Given that currently the implementation allows malformed
>>> and unrecognized tunables pass through, it may even allow SXID_ERASE
>>> tunables to go through.
>>
>> I have read through bug 27471, thanks for referencing the bug.
>>   I'm worried about the fact that we have existing examples from DJ's
>> nsswitch changes with / changing, that there are real-world examples
>> where we actively run a child process in a potentially newer glibc
>> and stripping away tunables for the current hardware means the child
>> process needs a way to add it back, and doing that isn't entirely
>> clear e.g. sudo with hardcoded env.
>>
>> This makes testing and producing tuning difficult or impossible
>> and I that means to me that we should be more conservative with what
>> we process.
>>
>>> This change revamps the SXID_ERASE implementation so that:
>>>
>>> - Only valid tunables are written back to the tunestr string, because
>>>    of which children of SXID programs will only inherit a clean list of
>>>    identified tunables that are not SXID_ERASE.
>>
>> This prevents passing a tunable through a program to a child that runs
>> with a newer glibc and accepts different tunables.
>>
>>> - Unrecognized tunables get scrubbed off from the environment and
>>>    subsequently from the child environment.
>>
>> I think we need to be conservative here and allow unknown tunables
>> to be "passed through" to the child without modification.
>>
>>> - This has the side-effect that a tunable that is not identified by
>>>    the setxid binary, will not be passed on to a non-setxid child even
>>>    if the child could have identified that tunable.  This may break
>>>    applications that expect this behaviour but expecting such tunables
>>>    to cross the SXID boundary is wrong.
>>
>> Why is it wrong?
> 
> That comment was based on a previous consensus[1] where we agreed
> that we eventually port all SXID_IGNORE tunables into SXID_ERASE but
> I never actually got around to doing it.  However you seem to arguing
> the opposite case, i.e. making tunables SXID_IGNORE by default and
> porting SXID_ERASE over to it.

If a deprecated SXID_IGNORE variable existed, which it doesn't, then
I would have expected it to cross the boundary.

I'm not arguing about defaults just behaviour of existing code and
how it might be used by users.

My argument is that it is not "wrong" for tunables to cross the SXID
boundary if they were marked as being allowed to cross that boundary.

However, I need to review this patch again because:

* By default we set tunables to SXID_ERASE.

* We have no deprecated SXID_IGNORE tunables that I know about.

If we had the latter then we would need to do more work to keep them,
but we don't so your patch might be OK.
 
> As I've mentioned in the bug report, I don't see miscategorization of
> tunables resulting in any *new* security issues; at worst they may
> provide additional control over processes across setxid boundaries
> that may otherwise not be possible.  In that sense, sxid
> categorization is a security hardening feature and the discussion is
> about whether the hardening is going too far.

The hardening, SXID_ERASE, should follow the documented behaviour
for a given variable, including deprecated ones.

>> We can have instances where we chroot or unshare into another glibc
>> version and we may want the env vars to pass through because they are
>> related to hardware changes.
>>
>> If I step back and look at the bigger picture I see the following:
>>
>> (1) Tunables we have removed.
>> - Did we record these?
>> - Did we record their security setting?
>> - Are we avoiding reusing them by accident?
>> - Are we cleaning legacy tunables based on their security setting to
>>    keep the environment clean for a child running with an old glibc?
> 
> There was one namespace renaming from glibc.tune to glibc.cpu, but it
> was aarch64-specific.  It defaults to SXID_ERASE, as do most
> tunables. Only tunables that were ported from environment variables,
> e.g. glibc.malloc.mmap_threshold are SXID_IGNORE.

Right, which is why today your patch should be OK, and I need to redo
the review.

> 
>> (2) Tunables we don't know about.
>> - The child process may need to consume these because it runs a newer glibc.
>> - We lack /var/glibc.tunables handling of these tunables, or the child may be a
>>    generic container, or generic chroot, maybe env vars are the only way to pass
>>    this information.
>>
>> (3) Tunables we know about.
>> - These are the straight forward fix, we just fix the code.
>>
>> The patch handles (1) and (2) and (3) but picks an easier to audit
>> and verify solution for (3) at the expense of (1) and (2). I don't know if
>> that is a good tradeoff.
>>
>> With my developer hat on I expect the following:
>> - All old legacy tunables are handled as their old security settings.
>> - All new tunables are handled as their current security settings.
>> - Unknown tunables are left alone.
>>
>> Have I justified my expectation?
> 
> To summarize my understanding, you argue that there may be a
> legitimate need to be able to control environment of processes across
> the sxid boundary.  While I agree that there ought to be ways to
> control processes across sxid boundaries I don't think it should be
> as trivial as setting the program environment of an sxid program.

I think it should.

Setting values in a file for a container or cross-mount-namespace
requires considerable amount of work.

In k8s and even Red Hat OCP there is *significant* use of environment
variables to express tunables for containers.

Consider a read-only runtime... how do you impact the behaviour? How
do you test a tuning to a hardware feature that only the container
runtime knows about?

In practice I'd like the tuning to be automatic e.g. ifunc, but some
options are difficult and workload specific, and so may need to pass
to a child process across an SXID boundary.

It is *more* dangerous IMO to agree to mount a read-write filesystem
into which you have to write configuration data.

However, I don't think this discussion should or needs to be resolved
today since we have no deprecated SXID_IGNORE env vars that would need
special handling.
 
> Maybe the use case you're referring to is better served by systemwide
> and userwide tunables.  For example, if I had to control the
> environment of httpd, it should be through
> $HTTPD_HOME/.glibc-tunables or /etc/glibc-tunables and not through
> $SIDDHESH_HOME/.glibc-tunables or the environment of user siddhesh.
> This may not work for legacy tunables if the older environment
> doesn't support systemwide or userwide tunables but that shouldn't
> cause problems in practice.

Some tunables I think can be set in that way because you have:

* A known workload tuning.

* A known parameter for your runtime.

But there are much harder parameters to set that depend on the runtime
knowing something more about the host. Today this happens to be
environment variables.

> [1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
> setuid programs, it ends up setting the envvar string iterator
> incorrectly, because of which it may parse the next tunable
> incorrectly.  Given that currently the implementation allows malformed
> and unrecognized tunables pass through, it may even allow SXID_ERASE
> tunables to go through.

I am reviewing this a second time after our downthread discussion that
there are no SXID_IGNORE tunables that are deprecated and that would
actually need to be handled in a special way. Any deprecated SXID_ERASE
variables would be unknown to a later runtime and be erased anyway
matching the SXID_ERASE requirement.
 
> This change revamps the SXID_ERASE implementation so that:
> 
> - Only valid tunables are written back to the tunestr string, because
>   of which children of SXID programs will only inherit a clean list of
>   identified tunables that are not SXID_ERASE.
> 
> - Unrecognized tunables get scrubbed off from the environment and
>   subsequently from the child environment.
> 
> - This has the side-effect that a tunable that is not identified by
>   the setxid binary, will not be passed on to a non-setxid child even
>   if the child could have identified that tunable.  This may break
>   applications that expect this behaviour but expecting such tunables
>   to cross the SXID boundary is wrong.

Re-review in the context of a limited change to fix the bug.

OK to commit.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
>  elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
>  2 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..1aedb9bd36 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring)
>      return;
>  
>    char *p = tunestr;
> +  size_t off = 0;

OK.

>  
>    while (true)
>      {
> @@ -184,7 +185,11 @@ parse_tunables (char *tunestr, char *valstring)
>        /* If we reach the end of the string before getting a valid name-value
>  	 pair, bail out.  */
>        if (p[len] == '\0')
> -	return;
> +	{
> +	  if (__libc_enable_secure)
> +	    tunestr[off] = '\0';
> +	  return;

OK. Terminate the tunable if we are secure.

> +	}
>  
>        /* We did not find a valid name-value pair before encountering the
>  	 colon.  */
> @@ -210,35 +215,28 @@ parse_tunables (char *tunestr, char *valstring)
>  
>  	  if (tunable_is_name (cur->name, name))
>  	    {
> -	      /* If we are in a secure context (AT_SECURE) then ignore the tunable
> -		 unless it is explicitly marked as secure.  Tunable values take
> -		 precedence over their envvar aliases.  */
> +	      /* If we are in a secure context (AT_SECURE) then ignore the
> +		 tunable unless it is explicitly marked as secure.  Tunable
> +		 values take precedence over their envvar aliases.  We write
> +		 the tunables that are not SXID_ERASE back to TUNESTR, thus
> +		 dropping all SXID_ERASE tunables and any invalid or
> +		 unrecognized tunables.  */

OK. For today there are no SXID_IGNORE tunables that have been deprecated, and so
all currently deprecated tunables were always SXID_ERASE and so this doesn't
change the current behaviour. Future changes will need to take this into account.

>  	      if (__libc_enable_secure)
>  		{
> -		  if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> +		  if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>  		    {
> -		      if (p[len] == '\0')
> -			{
> -			  /* Last tunable in the valstring.  Null-terminate and
> -			     return.  */
> -			  *name = '\0';
> -			  return;
> -			}
> -		      else
> -			{
> -			  /* Remove the current tunable from the string.  We do
> -			     this by overwriting the string starting from NAME
> -			     (which is where the current tunable begins) with
> -			     the remainder of the string.  We then have P point
> -			     to NAME so that we continue in the correct
> -			     position in the valstring.  */
> -			  char *q = &p[len + 1];
> -			  p = name;
> -			  while (*q != '\0')
> -			    *name++ = *q++;
> -			  name[0] = '\0';
> -			  len = 0;
> -			}
> +		      if (off > 0)
> +			tunestr[off++] = ':';
> +
> +		      const char *n = cur->name;
> +
> +		      while (*n != '\0')
> +			tunestr[off++] = *n++;
> +
> +		      tunestr[off++] = '=';
> +
> +		      for (size_t j = 0; j < len; j++)
> +			tunestr[off++] = value[j];

OK.

>  		    }
>  
>  		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> @@ -251,9 +249,7 @@ parse_tunables (char *tunestr, char *valstring)
>  	    }
>  	}
>  
> -      if (p[len] == '\0')
> -	return;
> -      else
> +      if (p[len] != '\0')

OK.

>  	p += len + 1;
>      }
>  }
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 3d523875b1..05619c9adc 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -45,11 +45,37 @@
>  const char *teststrings[] =
>  {
>    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> +  "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
> +  ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> +  "glibc.malloc.check=1:glibc.malloc.check=2",
> +  "not_valid.malloc.check=2",
> +  "glibc.not_valid.check=2",
>  };
>  
>  const char *resultstrings[] =
>  {
>    "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",

OK. Adds many more tests, which is great!

>  };
>  
>  static int
>