Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
> > On 23 Feb 2018, at 11:31, Christophe Fergeau wrote: > > > > On Fri, Feb 23, 2018 at 10:11:46AM +, Frediano Ziglio wrote: > >> Depending on how structures are initialised in the code is > >> possible that implicit padding bytes are not initialised > >> causing possible information leaks as the entire structure > >> with all padding is sent through device/network. > >> > >> Signed-off-by: Frediano Ziglio > >> --- > >> spice/stream-device.h | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/spice/stream-device.h b/spice/stream-device.h > >> index 2e7c50e..b2f83b5 100644 > >> --- a/spice/stream-device.h > >> +++ b/spice/stream-device.h > >> @@ -48,6 +48,8 @@ > >> * containing integers up to 64 bit. > >> * All numbers are in little endian format. > >> * > >> + * For security reasons structures should not contain implicit paddings. > >> + * > > > > Isn't padding inserted by the compiler going to be platform-dependent > > anyway? > > That is my concern too. > Yes, every time you send data to a stream you are doing platform-dependent implications (unless the transport send all bits and you have same ABIs/encodings at both ends). > > I would say that all structures used in the protocol should be packed. > Yes, actually the "naturally aligned" requires it. Is a way to avoid misalignments. The transportation in this case is supposed to be fast the space occupied by padding is not an issue. Considering that when TCP was designed machines had few MBs and still they cared about alignment I don't see why should ignore in a fast transport. One could complain about the endian and ask for a more guest friendly as having the 2 ends with different endian here could mean full emulation where doing endian conversion in the guest could be expensive, on the other hand a protocol supporting both endianness could have some performance impact and we mostly support little endian machines. > I would also specify that what is sent is little-endian. While on x86, there > is only one endianness, some platforms, e.g. Itanium, are bi-endian, so it > is theoretically possible for the host to be big and the guest to be little > for example. OK, I know, Itanium… :-) > There's a "All numbers are in little endian format.". I didn't specify the byte has 8 bit and the transport can carry full octets but I think this is too paranoid. > > > > Christophe > > > >> * The protocol can be defined by these states: > >> * - Initial. Device just opened. Guest should wait > >> * for a message from the host; Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
> > > On 23 Feb 2018, at 11:11, Frediano Ziglio wrote: > > > > Depending on how structures are initialised in the code is > > possible that implicit padding bytes are not initialised > > causing possible information leaks as the entire structure > > with all padding is sent through device/network. > > > > Signed-off-by: Frediano Ziglio > > --- > > spice/stream-device.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/spice/stream-device.h b/spice/stream-device.h > > index 2e7c50e..b2f83b5 100644 > > --- a/spice/stream-device.h > > +++ b/spice/stream-device.h > > @@ -48,6 +48,8 @@ > > * containing integers up to 64 bit. > > * All numbers are in little endian format. > > * > > + * For security reasons structures should not contain implicit paddings. > > Acked-by: Christophe de Dinechin > > > + * > > * The protocol can be defined by these states: > > * - Initial. Device just opened. Guest should wait > > * for a message from the host; I actually nack myself. "naturally aligned" already requires this so this is not adding much but only confusing, unless was an explanation for the terminology which indeed is not common. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
> On 23 Feb 2018, at 11:31, Christophe Fergeau wrote: > > On Fri, Feb 23, 2018 at 10:11:46AM +, Frediano Ziglio wrote: >> Depending on how structures are initialised in the code is >> possible that implicit padding bytes are not initialised >> causing possible information leaks as the entire structure >> with all padding is sent through device/network. >> >> Signed-off-by: Frediano Ziglio >> --- >> spice/stream-device.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/spice/stream-device.h b/spice/stream-device.h >> index 2e7c50e..b2f83b5 100644 >> --- a/spice/stream-device.h >> +++ b/spice/stream-device.h >> @@ -48,6 +48,8 @@ >> * containing integers up to 64 bit. >> * All numbers are in little endian format. >> * >> + * For security reasons structures should not contain implicit paddings. >> + * > > Isn't padding inserted by the compiler going to be platform-dependent anyway? That is my concern too. > I would say that all structures used in the protocol should be packed. I would also specify that what is sent is little-endian. While on x86, there is only one endianness, some platforms, e.g. Itanium, are bi-endian, so it is theoretically possible for the host to be big and the guest to be little for example. OK, I know, Itanium… :-) > > Christophe > >> * The protocol can be defined by these states: >> * - Initial. Device just opened. Guest should wait >> * for a message from the host; >> -- >> 2.14.3 >> >> ___ >> Spice-devel mailing list >> Spice-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/spice-devel > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
On Fri, Feb 23, 2018 at 10:11:46AM +, Frediano Ziglio wrote: > Depending on how structures are initialised in the code is > possible that implicit padding bytes are not initialised > causing possible information leaks as the entire structure > with all padding is sent through device/network. > > Signed-off-by: Frediano Ziglio > --- > spice/stream-device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/spice/stream-device.h b/spice/stream-device.h > index 2e7c50e..b2f83b5 100644 > --- a/spice/stream-device.h > +++ b/spice/stream-device.h > @@ -48,6 +48,8 @@ > * containing integers up to 64 bit. > * All numbers are in little endian format. > * > + * For security reasons structures should not contain implicit paddings. > + * Isn't padding inserted by the compiler going to be platform-dependent anyway? I would say that all structures used in the protocol should be packed. Christophe > * The protocol can be defined by these states: > * - Initial. Device just opened. Guest should wait > * for a message from the host; > -- > 2.14.3 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
On that topic, curious why the structures are “naturally aligned”? Why aren’t they packed? > On 23 Feb 2018, at 11:11, Frediano Ziglio wrote: > > Depending on how structures are initialised in the code is > possible that implicit padding bytes are not initialised > causing possible information leaks as the entire structure > with all padding is sent through device/network. > > Signed-off-by: Frediano Ziglio > --- > spice/stream-device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/spice/stream-device.h b/spice/stream-device.h > index 2e7c50e..b2f83b5 100644 > --- a/spice/stream-device.h > +++ b/spice/stream-device.h > @@ -48,6 +48,8 @@ > * containing integers up to 64 bit. > * All numbers are in little endian format. > * > + * For security reasons structures should not contain implicit paddings. > + * > * The protocol can be defined by these states: > * - Initial. Device just opened. Guest should wait > * for a message from the host; > -- > 2.14.3 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
> On 23 Feb 2018, at 11:11, Frediano Ziglio wrote: > > Depending on how structures are initialised in the code is > possible that implicit padding bytes are not initialised > causing possible information leaks as the entire structure > with all padding is sent through device/network. > > Signed-off-by: Frediano Ziglio > --- > spice/stream-device.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/spice/stream-device.h b/spice/stream-device.h > index 2e7c50e..b2f83b5 100644 > --- a/spice/stream-device.h > +++ b/spice/stream-device.h > @@ -48,6 +48,8 @@ > * containing integers up to 64 bit. > * All numbers are in little endian format. > * > + * For security reasons structures should not contain implicit paddings. Acked-by: Christophe de Dinechin > + * > * The protocol can be defined by these states: > * - Initial. Device just opened. Guest should wait > * for a message from the host; > -- > 2.14.3 > > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures
Depending on how structures are initialised in the code is possible that implicit padding bytes are not initialised causing possible information leaks as the entire structure with all padding is sent through device/network. Signed-off-by: Frediano Ziglio --- spice/stream-device.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spice/stream-device.h b/spice/stream-device.h index 2e7c50e..b2f83b5 100644 --- a/spice/stream-device.h +++ b/spice/stream-device.h @@ -48,6 +48,8 @@ * containing integers up to 64 bit. * All numbers are in little endian format. * + * For security reasons structures should not contain implicit paddings. + * * The protocol can be defined by these states: * - Initial. Device just opened. Guest should wait * for a message from the host; -- 2.14.3 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
