[PATCH] nss_dns: Check for proper A/AAAA address alignment

[PATCH] nss_dns: Check for proper A/AAAA address alignment

[Florian Weimer]

2019-05-16  Florian Weimer  <fweimer@redhat.com>

	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
	struct in_addr/struct in6_addr alignment.

diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index 9c15f25f28..9c40051aff 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -947,8 +947,15 @@ getanswer_r (struct resolv_context *ctx,
 	      linebuflen -= nn;
 	    }
 
-	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
-	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
+	  /* Provide sufficient alignment for both address
+	     families.  */
+	  enum { align = 4 };
+	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
+			  "struct in_addr alignment");
+	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
+			  "struct in6_addr alignment");
+	  linebuflen -= align - ((u_long) bp % align);
+	  bp += align - ((u_long) bp % align);
 
 	  if (__glibc_unlikely (n > linebuflen))
 	    goto too_small;

Re: [PATCH] nss_dns: Check for proper A/AAAA address alignment

[Carlos O'Donell]

On 5/16/19 1:18 PM, Florian Weimer wrote:
> 2019-05-16  Florian Weimer  <fweimer@redhat.com>
> 
> 	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
> 	struct in_addr/struct in6_addr alignment.
> 

Can we use standard macros to compute alignmnet and differences, it
makes the code more easy to read at a glance.

> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index 9c15f25f28..9c40051aff 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -947,8 +947,15 @@ getanswer_r (struct resolv_context *ctx,
>  	      linebuflen -= nn;
>  	    }
>  
> -	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
> -	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
> +	  /* Provide sufficient alignment for both address
> +	     families.  */
> +	  enum { align = 4 };
> +	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
> +			  "struct in_addr alignment");
> +	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
> +			  "struct in6_addr alignment");

OK.

> +	  linebuflen -= align - ((u_long) bp % align);
> +	  bp += align - ((u_long) bp % align);

Is the use case common enough to add something to libc-pointer-arith.h
to handle linebuflen adjustment?

e.g.

align_diff = ALIGN_UP_DIFF (bp, align);
linebuflen -= align_diff;
bp += align_diff;

If not then can we still use ALIGN_UP? It makes it immediately
obvious the intent was to align the pointer upwards and adjust
the length of the remaining buffer.

>  
>  	  if (__glibc_unlikely (n > linebuflen))
>  	    goto too_small;
> 


-- 
Cheers,
Carlos.

Re: [PATCH] nss_dns: Check for proper A/AAAA address alignment

[Florian Weimer]

* Carlos O'Donell:

> On 5/16/19 1:18 PM, Florian Weimer wrote:
>> 2019-05-16  Florian Weimer  <fweimer@redhat.com>
>> 
>> 	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
>> 	struct in_addr/struct in6_addr alignment.
>> 
>
> Can we use standard macros to compute alignmnet and differences, it
> makes the code more easy to read at a glance.

I want to convert this to struct alloc_buffer eventually, then this will
go away anyway.  I'm trying to post smaller patches towards this goal.
These changes are really hard to review unfortunately.

As a stop-gap measure, I've changed the code to use PTR_ALIGN_UP.

>> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
>> index 9c15f25f28..9c40051aff 100644
>> --- a/resolv/nss_dns/dns-host.c
>> +++ b/resolv/nss_dns/dns-host.c
>> @@ -947,8 +947,15 @@ getanswer_r (struct resolv_context *ctx,
>>  	      linebuflen -= nn;
>>  	    }
>>  
>> -	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
>> -	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
>> +	  /* Provide sufficient alignment for both address
>> +	     families.  */
>> +	  enum { align = 4 };
>> +	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
>> +			  "struct in_addr alignment");
>> +	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
>> +			  "struct in6_addr alignment");
>
> OK.
>
>> +	  linebuflen -= align - ((u_long) bp % align);
>> +	  bp += align - ((u_long) bp % align);
>
> Is the use case common enough to add something to libc-pointer-arith.h
> to handle linebuflen adjustment?

Yes, see struct alloc_buffer.

> align_diff = ALIGN_UP_DIFF (bp, align);
> linebuflen -= align_diff;
> bp += align_diff;
>
> If not then can we still use ALIGN_UP? It makes it immediately
> obvious the intent was to align the pointer upwards and adjust
> the length of the remaining buffer.

This lacks overflow checking.  It is not a good coding pattern in my
opinion.

Thanks,
Florian

nss_dns: Check for proper A/AAAA address alignment

2019-05-24  Florian Weimer  <fweimer@redhat.com>

	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
	struct in_addr/struct in6_addr alignment.

diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
index 9c15f25f28..5af47fd10d 100644
--- a/resolv/nss_dns/dns-host.c
+++ b/resolv/nss_dns/dns-host.c
@@ -78,6 +78,7 @@
 #include <stdlib.h>
 #include <stddef.h>
 #include <string.h>
+#include <libc-pointer-arith.h>
 
 #include "nsswitch.h"
 #include <arpa/nameser.h>
@@ -947,8 +948,18 @@ getanswer_r (struct resolv_context *ctx,
 	      linebuflen -= nn;
 	    }
 
-	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
-	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
+	  /* Provide sufficient alignment for both address
+	     families.  */
+	  enum { align = 4 };
+	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
+			  "struct in_addr alignment");
+	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
+			  "struct in6_addr alignment");
+	  {
+	    char *new_bp = PTR_ALIGN_UP (bp, align);
+	    linebuflen -= new_bp - bp;
+	    bp = new_bp;
+	  }
 
 	  if (__glibc_unlikely (n > linebuflen))
 	    goto too_small;

Re: [PATCH] nss_dns: Check for proper A/AAAA address alignment

[Carlos O'Donell]

On 5/24/19 2:27 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 5/16/19 1:18 PM, Florian Weimer wrote:
>>> 2019-05-16  Florian Weimer  <fweimer@redhat.com>
>>>
>>> 	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
>>> 	struct in_addr/struct in6_addr alignment.
>>>
>>
>> Can we use standard macros to compute alignmnet and differences, it
>> makes the code more easy to read at a glance.
> 
> I want to convert this to struct alloc_buffer eventually, then this will
> go away anyway.  I'm trying to post smaller patches towards this goal.
> These changes are really hard to review unfortunately.

That makes perfect sense.

> As a stop-gap measure, I've changed the code to use PTR_ALIGN_UP.
> 
>>> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
>>> index 9c15f25f28..9c40051aff 100644
>>> --- a/resolv/nss_dns/dns-host.c
>>> +++ b/resolv/nss_dns/dns-host.c
>>> @@ -947,8 +947,15 @@ getanswer_r (struct resolv_context *ctx,
>>>  	      linebuflen -= nn;
>>>  	    }
>>>  
>>> -	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
>>> -	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
>>> +	  /* Provide sufficient alignment for both address
>>> +	     families.  */
>>> +	  enum { align = 4 };
>>> +	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
>>> +			  "struct in_addr alignment");
>>> +	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
>>> +			  "struct in6_addr alignment");
>>
>> OK.
>>
>>> +	  linebuflen -= align - ((u_long) bp % align);
>>> +	  bp += align - ((u_long) bp % align);
>>
>> Is the use case common enough to add something to libc-pointer-arith.h
>> to handle linebuflen adjustment?
> 
> Yes, see struct alloc_buffer.

Good point.

>> align_diff = ALIGN_UP_DIFF (bp, align);
>> linebuflen -= align_diff;
>> bp += align_diff;
>>
>> If not then can we still use ALIGN_UP? It makes it immediately
>> obvious the intent was to align the pointer upwards and adjust
>> the length of the remaining buffer.
> 
> This lacks overflow checking.  It is not a good coding pattern in my
> opinion.

Also a good point. I assumed, but hadn't checked that this didn't
have overflow checking.

Yes, from a "pattern" perspective it's better to use some kind of
buffer managment API like alloc_buffer.

> Thanks,
> Florian
> 
> nss_dns: Check for proper A/AAAA address alignment
> 
> 2019-05-24  Florian Weimer  <fweimer@redhat.com>
> 
> 	* resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about
> 	struct in_addr/struct in6_addr alignment.
> 

New version looks good to me.

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

> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c
> index 9c15f25f28..5af47fd10d 100644
> --- a/resolv/nss_dns/dns-host.c
> +++ b/resolv/nss_dns/dns-host.c
> @@ -78,6 +78,7 @@
>  #include <stdlib.h>
>  #include <stddef.h>
>  #include <string.h>
> +#include <libc-pointer-arith.h>

OK.

>  
>  #include "nsswitch.h"
>  #include <arpa/nameser.h>
> @@ -947,8 +948,18 @@ getanswer_r (struct resolv_context *ctx,
>  	      linebuflen -= nn;
>  	    }
>  
> -	  linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align));
> -	  bp += sizeof (align) - ((u_long) bp % sizeof (align));
> +	  /* Provide sufficient alignment for both address
> +	     families.  */
> +	  enum { align = 4 };
> +	  _Static_assert ((align % __alignof__ (struct in_addr)) == 0,
> +			  "struct in_addr alignment");
> +	  _Static_assert ((align % __alignof__ (struct in6_addr)) == 0,
> +			  "struct in6_addr alignment");
> +	  {
> +	    char *new_bp = PTR_ALIGN_UP (bp, align);
> +	    linebuflen -= new_bp - bp;
> +	    bp = new_bp;
> +	  }

OK.

>  
>  	  if (__glibc_unlikely (n > linebuflen))
>  	    goto too_small;
> 


-- 
Cheers,
Carlos.