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