alignment issue using sctp_assoc_stats via getsockopt

alignment issue using sctp_assoc_stats via getsockopt

[Jakub Tomalak]

Greetings,

I have discovered an alignment issue when using SCTP in a 32-bit
binary on a 64-bit host.

Essentially, the problem is that struct sctp_assoc_stats contains a
sockaddr_storage field and is at a different offset for 32-bit and
64-bit architectures (well, Intel/AMD anyway).

Essentially when a 32-bit binary makes a getsockopt call with
sctp_assoc_stats againts a 64-bit kernel, the kernel copies the data
in with a different alignment, causing the 32-bit binary to receive
incorrect data for all of the chunk counters (fields after the
sockaddr field)..

The fix is relatively simple for i386 and x86_64 architectures. I am
not sure about MIPS64 and ARM, since a colleague mentioned they do not
support unaligned memory access and also mentioned that MIPS64 doesn't
allow alignment with word sizes smaller than 8 bytes.

Here is the diff I tested against both 32-bit and 64-bit RHEL and
Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index ce70fe6..41aa566 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -873,7 +873,7 @@ struct sctp_assoc_stats {
        __u64           sas_iodchunks;   /* Ordered data chunks received */
        __u64           sas_octrlchunks; /* Control chunks sent */
        __u64           sas_ictrlchunks; /* Control chunks received */
-};
+} __attribute__((packed, aligned(4)));

/*
  * 8.1 sctp_bindx()
@@ -900,6 +900,6 @@ struct sctp_paddrthlds {
        struct sockaddr_storage spt_address;
        __u16 spt_pathmaxrxt;
        __u16 spt_pathpfthld;
-};
+} __attribute__((packed, aligned(4)));

#endif /* _UAPI_SCTP_H */

I have no idea what the official process is for submitting patches to
the SCTP module, and since this is probably a very edge case scenario,
I thought I would at least mention it in case anyone else comes across
similar problems.

Best Regards,
Jaub Tomalak

Re: alignment issue using sctp_assoc_stats via getsockopt

[Marcelo Ricardo Leitner]

Hi Jakub,

On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
> Greetings,
> 
> I have discovered an alignment issue when using SCTP in a 32-bit
> binary on a 64-bit host.
> 
> Essentially, the problem is that struct sctp_assoc_stats contains a
> sockaddr_storage field and is at a different offset for 32-bit and
> 64-bit architectures (well, Intel/AMD anyway).
> 
> Essentially when a 32-bit binary makes a getsockopt call with
> sctp_assoc_stats againts a 64-bit kernel, the kernel copies the data
> in with a different alignment, causing the 32-bit binary to receive
> incorrect data for all of the chunk counters (fields after the
> sockaddr field)..
> 
> The fix is relatively simple for i386 and x86_64 architectures. I am
> not sure about MIPS64 and ARM, since a colleague mentioned they do not
> support unaligned memory access and also mentioned that MIPS64 doesn't
> allow alignment with word sizes smaller than 8 bytes.
> 
> Here is the diff I tested against both 32-bit and 64-bit RHEL and
> Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index ce70fe6..41aa566 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
>         __u64           sas_iodchunks;   /* Ordered data chunks received */
>         __u64           sas_octrlchunks; /* Control chunks sent */
>         __u64           sas_ictrlchunks; /* Control chunks received */
> -};
> +} __attribute__((packed, aligned(4)));
> 
> /*
>   * 8.1 sctp_bindx()
> @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
>         struct sockaddr_storage spt_address;
>         __u16 spt_pathmaxrxt;
>         __u16 spt_pathpfthld;
> -};
> +} __attribute__((packed, aligned(4)));
> 
> #endif /* _UAPI_SCTP_H */
> 
> I have no idea what the official process is for submitting patches to
> the SCTP module, and since this is probably a very edge case scenario,
> I thought I would at least mention it in case anyone else comes across
> similar problems.

We can say that it starts by sharing your thoughts like you did. And
when you are confident with a patch, you can submit it to netdev@
directly while keeping this ML Cc'ed. Dave Miller is the one who
integrates sctp patches.

Now about your patch. I think it's just too late to fix that.  That
issue is exposed to user-space for too long so that I'm afraid it has
already become a "feature". If we fix that, it will break applications
that are handling that in some other way.

  Marcelo

Re: alignment issue using sctp_assoc_stats via getsockopt

[Neil Horman]

On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner wrote:
> Hi Jakub,
> 
> On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
> > Greetings,
> > 
> > I have discovered an alignment issue when using SCTP in a 32-bit
> > binary on a 64-bit host.
> > 
> > Essentially, the problem is that struct sctp_assoc_stats contains a
> > sockaddr_storage field and is at a different offset for 32-bit and
> > 64-bit architectures (well, Intel/AMD anyway).
> > 
> > Essentially when a 32-bit binary makes a getsockopt call with
> > sctp_assoc_stats againts a 64-bit kernel, the kernel copies the data
> > in with a different alignment, causing the 32-bit binary to receive
> > incorrect data for all of the chunk counters (fields after the
> > sockaddr field)..
> > 
> > The fix is relatively simple for i386 and x86_64 architectures. I am
> > not sure about MIPS64 and ARM, since a colleague mentioned they do not
> > support unaligned memory access and also mentioned that MIPS64 doesn't
> > allow alignment with word sizes smaller than 8 bytes.
> > 
> > Here is the diff I tested against both 32-bit and 64-bit RHEL and
> > Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
> > 
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index ce70fe6..41aa566 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
> >         __u64           sas_iodchunks;   /* Ordered data chunks received */
> >         __u64           sas_octrlchunks; /* Control chunks sent */
> >         __u64           sas_ictrlchunks; /* Control chunks received */
> > -};
> > +} __attribute__((packed, aligned(4)));
> > 
> > /*
> >   * 8.1 sctp_bindx()
> > @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
> >         struct sockaddr_storage spt_address;
> >         __u16 spt_pathmaxrxt;
> >         __u16 spt_pathpfthld;
> > -};
> > +} __attribute__((packed, aligned(4)));
> > 
> > #endif /* _UAPI_SCTP_H */
> > 
> > I have no idea what the official process is for submitting patches to
> > the SCTP module, and since this is probably a very edge case scenario,
> > I thought I would at least mention it in case anyone else comes across
> > similar problems.
> 
> We can say that it starts by sharing your thoughts like you did. And
> when you are confident with a patch, you can submit it to netdev@
> directly while keeping this ML Cc'ed. Dave Miller is the one who
> integrates sctp patches.
> 
> Now about your patch. I think it's just too late to fix that.  That
> issue is exposed to user-space for too long so that I'm afraid it has
> already become a "feature". If we fix that, it will break applications
> that are handling that in some other way.
> 
>   Marcelo
> 
> --
For the life of me I can't recall how to do this, but can we add a thunk layer
to the syscall to detect the arch of the calling process and fix up the
alignment while inside the syscall, and translate back in the epiloge?

Neil

> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: alignment issue using sctp_assoc_stats via getsockopt

[Marcelo]

Em 8 de outubro de 2015 08:11:40 BRT, Neil Horman <nhorman@tuxdriver.com> escreveu:
>On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner
>wrote:
>> Hi Jakub,
>> 
>> On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
>> > Greetings,
>> > 
>> > I have discovered an alignment issue when using SCTP in a 32-bit
>> > binary on a 64-bit host.
>> > 
>> > Essentially, the problem is that struct sctp_assoc_stats contains a
>> > sockaddr_storage field and is at a different offset for 32-bit and
>> > 64-bit architectures (well, Intel/AMD anyway).
>> > 
>> > Essentially when a 32-bit binary makes a getsockopt call with
>> > sctp_assoc_stats againts a 64-bit kernel, the kernel copies the
>data
>> > in with a different alignment, causing the 32-bit binary to receive
>> > incorrect data for all of the chunk counters (fields after the
>> > sockaddr field)..
>> > 
>> > The fix is relatively simple for i386 and x86_64 architectures. I
>am
>> > not sure about MIPS64 and ARM, since a colleague mentioned they do
>not
>> > support unaligned memory access and also mentioned that MIPS64
>doesn't
>> > allow alignment with word sizes smaller than 8 bytes.
>> > 
>> > Here is the diff I tested against both 32-bit and 64-bit RHEL and
>> > Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
>> > 
>> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>> > index ce70fe6..41aa566 100644
>> > --- a/include/uapi/linux/sctp.h
>> > +++ b/include/uapi/linux/sctp.h
>> > @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
>> >         __u64           sas_iodchunks;   /* Ordered data chunks
>received */
>> >         __u64           sas_octrlchunks; /* Control chunks sent */
>> >         __u64           sas_ictrlchunks; /* Control chunks received
>*/
>> > -};
>> > +} __attribute__((packed, aligned(4)));
>> > 
>> > /*
>> >   * 8.1 sctp_bindx()
>> > @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
>> >         struct sockaddr_storage spt_address;
>> >         __u16 spt_pathmaxrxt;
>> >         __u16 spt_pathpfthld;
>> > -};
>> > +} __attribute__((packed, aligned(4)));
>> > 
>> > #endif /* _UAPI_SCTP_H */
>> > 
>> > I have no idea what the official process is for submitting patches
>to
>> > the SCTP module, and since this is probably a very edge case
>scenario,
>> > I thought I would at least mention it in case anyone else comes
>across
>> > similar problems.
>> 
>> We can say that it starts by sharing your thoughts like you did. And
>> when you are confident with a patch, you can submit it to netdev@
>> directly while keeping this ML Cc'ed. Dave Miller is the one who
>> integrates sctp patches.
>> 
>> Now about your patch. I think it's just too late to fix that.  That
>> issue is exposed to user-space for too long so that I'm afraid it has
>> already become a "feature". If we fix that, it will break
>applications
>> that are handling that in some other way.
>> 
>>   Marcelo
>> 
>> --
>For the life of me I can't recall how to do this, but can we add a
>thunk layer
>to the syscall to detect the arch of the calling process and fix up the
>alignment while inside the syscall, and translate back in the epiloge?
>
>Neil
>

Add this to lksctp-tools you mean?

>> To unsubscribe from this list: send the line "unsubscribe linux-sctp"
>in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

-- 
Sent from mobile. Please excuse my brevity.

Re: alignment issue using sctp_assoc_stats via getsockopt

[Neil Horman]

On Thu, Oct 08, 2015 at 08:26:56AM -0300, Marcelo wrote:
> 
> 
> Em 8 de outubro de 2015 08:11:40 BRT, Neil Horman <nhorman@tuxdriver.com> escreveu:
> >On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner
> >wrote:
> >> Hi Jakub,
> >> 
> >> On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
> >> > Greetings,
> >> > 
> >> > I have discovered an alignment issue when using SCTP in a 32-bit
> >> > binary on a 64-bit host.
> >> > 
> >> > Essentially, the problem is that struct sctp_assoc_stats contains a
> >> > sockaddr_storage field and is at a different offset for 32-bit and
> >> > 64-bit architectures (well, Intel/AMD anyway).
> >> > 
> >> > Essentially when a 32-bit binary makes a getsockopt call with
> >> > sctp_assoc_stats againts a 64-bit kernel, the kernel copies the
> >data
> >> > in with a different alignment, causing the 32-bit binary to receive
> >> > incorrect data for all of the chunk counters (fields after the
> >> > sockaddr field)..
> >> > 
> >> > The fix is relatively simple for i386 and x86_64 architectures. I
> >am
> >> > not sure about MIPS64 and ARM, since a colleague mentioned they do
> >not
> >> > support unaligned memory access and also mentioned that MIPS64
> >doesn't
> >> > allow alignment with word sizes smaller than 8 bytes.
> >> > 
> >> > Here is the diff I tested against both 32-bit and 64-bit RHEL and
> >> > Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
> >> > 
> >> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >> > index ce70fe6..41aa566 100644
> >> > --- a/include/uapi/linux/sctp.h
> >> > +++ b/include/uapi/linux/sctp.h
> >> > @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
> >> >         __u64           sas_iodchunks;   /* Ordered data chunks
> >received */
> >> >         __u64           sas_octrlchunks; /* Control chunks sent */
> >> >         __u64           sas_ictrlchunks; /* Control chunks received
> >*/
> >> > -};
> >> > +} __attribute__((packed, aligned(4)));
> >> > 
> >> > /*
> >> >   * 8.1 sctp_bindx()
> >> > @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
> >> >         struct sockaddr_storage spt_address;
> >> >         __u16 spt_pathmaxrxt;
> >> >         __u16 spt_pathpfthld;
> >> > -};
> >> > +} __attribute__((packed, aligned(4)));
> >> > 
> >> > #endif /* _UAPI_SCTP_H */
> >> > 
> >> > I have no idea what the official process is for submitting patches
> >to
> >> > the SCTP module, and since this is probably a very edge case
> >scenario,
> >> > I thought I would at least mention it in case anyone else comes
> >across
> >> > similar problems.
> >> 
> >> We can say that it starts by sharing your thoughts like you did. And
> >> when you are confident with a patch, you can submit it to netdev@
> >> directly while keeping this ML Cc'ed. Dave Miller is the one who
> >> integrates sctp patches.
> >> 
> >> Now about your patch. I think it's just too late to fix that.  That
> >> issue is exposed to user-space for too long so that I'm afraid it has
> >> already become a "feature". If we fix that, it will break
> >applications
> >> that are handling that in some other way.
> >> 
> >>   Marcelo
> >> 
> >> --
> >For the life of me I can't recall how to do this, but can we add a
> >thunk layer
> >to the syscall to detect the arch of the calling process and fix up the
> >alignment while inside the syscall, and translate back in the epiloge?
> >
> >Neil
> >
> 
> Add this to lksctp-tools you mean?
> 

No, to the kernel.  IIRC in the early days of x86_64, lots of syscalls had
problem like this, where 32 bit user space and 64 bit kernels didn't agree on
word sizes and alignment, so there was a thunk layer that detected that
condition, made a copy of the user passed data, fixed the alignment, and gave
that to the real syscall in the kernel, then reversed the process before
returning to user space.  It was horrid and messy, but it seems like we could do
something simmilar here

Neil

> >> To unsubscribe from this list: send the line "unsubscribe linux-sctp"
> >in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> 
> 
> -- 
> Sent from mobile. Please excuse my brevity.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: alignment issue using sctp_assoc_stats via getsockopt

[Daniel Borkmann]

On 10/08/2015 03:45 PM, Neil Horman wrote:
> On Thu, Oct 08, 2015 at 08:26:56AM -0300, Marcelo wrote:
>> Em 8 de outubro de 2015 08:11:40 BRT, Neil Horman <nhorman@tuxdriver.com> escreveu:
>>> On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner
>>> wrote:
>>>> Hi Jakub,
>>>>
>>>> On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
>>>>> Greetings,
>>>>>
>>>>> I have discovered an alignment issue when using SCTP in a 32-bit
>>>>> binary on a 64-bit host.
>>>>>
>>>>> Essentially, the problem is that struct sctp_assoc_stats contains a
>>>>> sockaddr_storage field and is at a different offset for 32-bit and
>>>>> 64-bit architectures (well, Intel/AMD anyway).
>>>>>
>>>>> Essentially when a 32-bit binary makes a getsockopt call with
>>>>> sctp_assoc_stats againts a 64-bit kernel, the kernel copies the
>>> data
>>>>> in with a different alignment, causing the 32-bit binary to receive
>>>>> incorrect data for all of the chunk counters (fields after the
>>>>> sockaddr field)..
>>>>>
>>>>> The fix is relatively simple for i386 and x86_64 architectures. I
>>> am
>>>>> not sure about MIPS64 and ARM, since a colleague mentioned they do
>>> not
>>>>> support unaligned memory access and also mentioned that MIPS64
>>> doesn't
>>>>> allow alignment with word sizes smaller than 8 bytes.
>>>>>
>>>>> Here is the diff I tested against both 32-bit and 64-bit RHEL and
>>>>> Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
>>>>>
>>>>> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>>>>> index ce70fe6..41aa566 100644
>>>>> --- a/include/uapi/linux/sctp.h
>>>>> +++ b/include/uapi/linux/sctp.h
>>>>> @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
>>>>>          __u64           sas_iodchunks;   /* Ordered data chunks
>>> received */
>>>>>          __u64           sas_octrlchunks; /* Control chunks sent */
>>>>>          __u64           sas_ictrlchunks; /* Control chunks received
>>> */
>>>>> -};
>>>>> +} __attribute__((packed, aligned(4)));
>>>>>
>>>>> /*
>>>>>    * 8.1 sctp_bindx()
>>>>> @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
>>>>>          struct sockaddr_storage spt_address;
>>>>>          __u16 spt_pathmaxrxt;
>>>>>          __u16 spt_pathpfthld;
>>>>> -};
>>>>> +} __attribute__((packed, aligned(4)));
>>>>>
>>>>> #endif /* _UAPI_SCTP_H */
>>>>>
>>>>> I have no idea what the official process is for submitting patches
>>> to
>>>>> the SCTP module, and since this is probably a very edge case
>>> scenario,
>>>>> I thought I would at least mention it in case anyone else comes
>>> across
>>>>> similar problems.
>>>>
>>>> We can say that it starts by sharing your thoughts like you did. And
>>>> when you are confident with a patch, you can submit it to netdev@
>>>> directly while keeping this ML Cc'ed. Dave Miller is the one who
>>>> integrates sctp patches.
>>>>
>>>> Now about your patch. I think it's just too late to fix that.  That
>>>> issue is exposed to user-space for too long so that I'm afraid it has
>>>> already become a "feature". If we fix that, it will break
>>> applications
>>>> that are handling that in some other way.
>>>>
>>>>    Marcelo
>>>>
>>>> --
>>> For the life of me I can't recall how to do this, but can we add a
>>> thunk layer
>>> to the syscall to detect the arch of the calling process and fix up the
>>> alignment while inside the syscall, and translate back in the epiloge?
>>>
>>> Neil
>>
>> Add this to lksctp-tools you mean?
>>
>
> No, to the kernel.  IIRC in the early days of x86_64, lots of syscalls had
> problem like this, where 32 bit user space and 64 bit kernels didn't agree on
> word sizes and alignment, so there was a thunk layer that detected that
> condition, made a copy of the user passed data, fixed the alignment, and gave
> that to the real syscall in the kernel, then reversed the process before
> returning to user space.  It was horrid and messy, but it seems like we could do
> something simmilar here

But for such things we have the compat infrastructure in the kernel, f.e.
see ffd5939381c6 ("net: sctp: fix sctp_connectx abi for ia32 emulation/compat
mode"). So, I think something similar for the getsockopt() might be required.

Cheers,
Daniel

Re: alignment issue using sctp_assoc_stats via getsockopt

[Neil Horman]

On Thu, Oct 08, 2015 at 04:06:59PM +0200, Daniel Borkmann wrote:
> On 10/08/2015 03:45 PM, Neil Horman wrote:
> >On Thu, Oct 08, 2015 at 08:26:56AM -0300, Marcelo wrote:
> >>Em 8 de outubro de 2015 08:11:40 BRT, Neil Horman <nhorman@tuxdriver.com> escreveu:
> >>>On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner
> >>>wrote:
> >>>>Hi Jakub,
> >>>>
> >>>>On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
> >>>>>Greetings,
> >>>>>
> >>>>>I have discovered an alignment issue when using SCTP in a 32-bit
> >>>>>binary on a 64-bit host.
> >>>>>
> >>>>>Essentially, the problem is that struct sctp_assoc_stats contains a
> >>>>>sockaddr_storage field and is at a different offset for 32-bit and
> >>>>>64-bit architectures (well, Intel/AMD anyway).
> >>>>>
> >>>>>Essentially when a 32-bit binary makes a getsockopt call with
> >>>>>sctp_assoc_stats againts a 64-bit kernel, the kernel copies the
> >>>data
> >>>>>in with a different alignment, causing the 32-bit binary to receive
> >>>>>incorrect data for all of the chunk counters (fields after the
> >>>>>sockaddr field)..
> >>>>>
> >>>>>The fix is relatively simple for i386 and x86_64 architectures. I
> >>>am
> >>>>>not sure about MIPS64 and ARM, since a colleague mentioned they do
> >>>not
> >>>>>support unaligned memory access and also mentioned that MIPS64
> >>>doesn't
> >>>>>allow alignment with word sizes smaller than 8 bytes.
> >>>>>
> >>>>>Here is the diff I tested against both 32-bit and 64-bit RHEL and
> >>>>>Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
> >>>>>
> >>>>>diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> >>>>>index ce70fe6..41aa566 100644
> >>>>>--- a/include/uapi/linux/sctp.h
> >>>>>+++ b/include/uapi/linux/sctp.h
> >>>>>@@ -873,7 +873,7 @@ struct sctp_assoc_stats {
> >>>>>         __u64           sas_iodchunks;   /* Ordered data chunks
> >>>received */
> >>>>>         __u64           sas_octrlchunks; /* Control chunks sent */
> >>>>>         __u64           sas_ictrlchunks; /* Control chunks received
> >>>*/
> >>>>>-};
> >>>>>+} __attribute__((packed, aligned(4)));
> >>>>>
> >>>>>/*
> >>>>>   * 8.1 sctp_bindx()
> >>>>>@@ -900,6 +900,6 @@ struct sctp_paddrthlds {
> >>>>>         struct sockaddr_storage spt_address;
> >>>>>         __u16 spt_pathmaxrxt;
> >>>>>         __u16 spt_pathpfthld;
> >>>>>-};
> >>>>>+} __attribute__((packed, aligned(4)));
> >>>>>
> >>>>>#endif /* _UAPI_SCTP_H */
> >>>>>
> >>>>>I have no idea what the official process is for submitting patches
> >>>to
> >>>>>the SCTP module, and since this is probably a very edge case
> >>>scenario,
> >>>>>I thought I would at least mention it in case anyone else comes
> >>>across
> >>>>>similar problems.
> >>>>
> >>>>We can say that it starts by sharing your thoughts like you did. And
> >>>>when you are confident with a patch, you can submit it to netdev@
> >>>>directly while keeping this ML Cc'ed. Dave Miller is the one who
> >>>>integrates sctp patches.
> >>>>
> >>>>Now about your patch. I think it's just too late to fix that.  That
> >>>>issue is exposed to user-space for too long so that I'm afraid it has
> >>>>already become a "feature". If we fix that, it will break
> >>>applications
> >>>>that are handling that in some other way.
> >>>>
> >>>>   Marcelo
> >>>>
> >>>>--
> >>>For the life of me I can't recall how to do this, but can we add a
> >>>thunk layer
> >>>to the syscall to detect the arch of the calling process and fix up the
> >>>alignment while inside the syscall, and translate back in the epiloge?
> >>>
> >>>Neil
> >>
> >>Add this to lksctp-tools you mean?
> >>
> >
> >No, to the kernel.  IIRC in the early days of x86_64, lots of syscalls had
> >problem like this, where 32 bit user space and 64 bit kernels didn't agree on
> >word sizes and alignment, so there was a thunk layer that detected that
> >condition, made a copy of the user passed data, fixed the alignment, and gave
> >that to the real syscall in the kernel, then reversed the process before
> >returning to user space.  It was horrid and messy, but it seems like we could do
> >something simmilar here
> 
> But for such things we have the compat infrastructure in the kernel, f.e.
> see ffd5939381c6 ("net: sctp: fix sctp_connectx abi for ia32 emulation/compat
> mode"). So, I think something similar for the getsockopt() might be required.
> 
Yes, thank you Daniel, thats what I was thinking of, not thunking
Neil

> Cheers,
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: alignment issue using sctp_assoc_stats via getsockopt

[Marcelo Ricardo Leitner]

Em 08-10-2015 11:07, Neil Horman escreveu:
> On Thu, Oct 08, 2015 at 04:06:59PM +0200, Daniel Borkmann wrote:
>> On 10/08/2015 03:45 PM, Neil Horman wrote:
>>> On Thu, Oct 08, 2015 at 08:26:56AM -0300, Marcelo wrote:
>>>> Em 8 de outubro de 2015 08:11:40 BRT, Neil Horman <nhorman@tuxdriver.com> escreveu:
>>>>> On Wed, Oct 07, 2015 at 06:11:55PM -0300, Marcelo Ricardo Leitner
>>>>> wrote:
>>>>>> Hi Jakub,
>>>>>>
>>>>>> On Tue, Oct 06, 2015 at 01:04:43PM +0200, Jakub Tomalak wrote:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> I have discovered an alignment issue when using SCTP in a 32-bit
>>>>>>> binary on a 64-bit host.
>>>>>>>
>>>>>>> Essentially, the problem is that struct sctp_assoc_stats contains a
>>>>>>> sockaddr_storage field and is at a different offset for 32-bit and
>>>>>>> 64-bit architectures (well, Intel/AMD anyway).
>>>>>>>
>>>>>>> Essentially when a 32-bit binary makes a getsockopt call with
>>>>>>> sctp_assoc_stats againts a 64-bit kernel, the kernel copies the
>>>>> data
>>>>>>> in with a different alignment, causing the 32-bit binary to receive
>>>>>>> incorrect data for all of the chunk counters (fields after the
>>>>>>> sockaddr field)..
>>>>>>>
>>>>>>> The fix is relatively simple for i386 and x86_64 architectures. I
>>>>> am
>>>>>>> not sure about MIPS64 and ARM, since a colleague mentioned they do
>>>>> not
>>>>>>> support unaligned memory access and also mentioned that MIPS64
>>>>> doesn't
>>>>>>> allow alignment with word sizes smaller than 8 bytes.
>>>>>>>
>>>>>>> Here is the diff I tested against both 32-bit and 64-bit RHEL and
>>>>>>> Ubuntu systems (with kernels ranging from 3.12.x to 3.16.y).
>>>>>>>
>>>>>>> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
>>>>>>> index ce70fe6..41aa566 100644
>>>>>>> --- a/include/uapi/linux/sctp.h
>>>>>>> +++ b/include/uapi/linux/sctp.h
>>>>>>> @@ -873,7 +873,7 @@ struct sctp_assoc_stats {
>>>>>>>          __u64           sas_iodchunks;   /* Ordered data chunks
>>>>> received */
>>>>>>>          __u64           sas_octrlchunks; /* Control chunks sent */
>>>>>>>          __u64           sas_ictrlchunks; /* Control chunks received
>>>>> */
>>>>>>> -};
>>>>>>> +} __attribute__((packed, aligned(4)));
>>>>>>>
>>>>>>> /*
>>>>>>>    * 8.1 sctp_bindx()
>>>>>>> @@ -900,6 +900,6 @@ struct sctp_paddrthlds {
>>>>>>>          struct sockaddr_storage spt_address;
>>>>>>>          __u16 spt_pathmaxrxt;
>>>>>>>          __u16 spt_pathpfthld;
>>>>>>> -};
>>>>>>> +} __attribute__((packed, aligned(4)));
>>>>>>>
>>>>>>> #endif /* _UAPI_SCTP_H */
>>>>>>>
>>>>>>> I have no idea what the official process is for submitting patches
>>>>> to
>>>>>>> the SCTP module, and since this is probably a very edge case
>>>>> scenario,
>>>>>>> I thought I would at least mention it in case anyone else comes
>>>>> across
>>>>>>> similar problems.
>>>>>>
>>>>>> We can say that it starts by sharing your thoughts like you did. And
>>>>>> when you are confident with a patch, you can submit it to netdev@
>>>>>> directly while keeping this ML Cc'ed. Dave Miller is the one who
>>>>>> integrates sctp patches.
>>>>>>
>>>>>> Now about your patch. I think it's just too late to fix that.  That
>>>>>> issue is exposed to user-space for too long so that I'm afraid it has
>>>>>> already become a "feature". If we fix that, it will break
>>>>> applications
>>>>>> that are handling that in some other way.
>>>>>>
>>>>>>    Marcelo
>>>>>>
>>>>>> --
>>>>> For the life of me I can't recall how to do this, but can we add a
>>>>> thunk layer
>>>>> to the syscall to detect the arch of the calling process and fix up the
>>>>> alignment while inside the syscall, and translate back in the epiloge?
>>>>>
>>>>> Neil
>>>>
>>>> Add this to lksctp-tools you mean?
>>>>
>>>
>>> No, to the kernel.  IIRC in the early days of x86_64, lots of syscalls had
>>> problem like this, where 32 bit user space and 64 bit kernels didn't agree on
>>> word sizes and alignment, so there was a thunk layer that detected that
>>> condition, made a copy of the user passed data, fixed the alignment, and gave
>>> that to the real syscall in the kernel, then reversed the process before
>>> returning to user space.  It was horrid and messy, but it seems like we could do
>>> something simmilar here
>>
>> But for such things we have the compat infrastructure in the kernel, f.e.
>> see ffd5939381c6 ("net: sctp: fix sctp_connectx abi for ia32 emulation/compat
>> mode"). So, I think something similar for the getsockopt() might be required.
>>
> Yes, thank you Daniel, thats what I was thinking of, not thunking

That still changes the ABI and will break applications that circumvented 
that, but yeah we have a case of success, I hope Dave will accept a 
similar fix for this one too. ;-)

   Marcelo