udf: Incorrect handling of timezone

udf: Incorrect handling of timezone

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

Hello! I looked at udf code which converts linux time to UDF time and I
found out that conversion of timezone is incorrect.

Relevant code from udf_time_to_disk_stamp() function:

	int16_t offset;

	offset = -sys_tz.tz_minuteswest;

	dest->typeAndTimezone = cpu_to_le16(0x1000 | (offset & 0x0FFF));

UDF 2.60 2.1.4.1 Uint16 TypeAndTimezone; says:

  For the following descriptions Type refers to the most significant 4 bits of this
  field, and TimeZone refers to the least significant 12 bits of this field, which is
  interpreted as a signed 12-bit number in two’s complement form.

  TimeZone ... If this field contains -2047 then the time zone has not been specified.

As offset is of signed 16bit integer, (offset & 0x0FFF) result always
clears sign bit and therefore timezone is stored to UDF fs incorrectly.

This needs to be fixed, sign bit from tz_minuteswest needs to be
propagated to 12th bit in typeAndTimezone member.

Also tz_minuteswest is of int type, so conversion to int16_t (or more
precisely int12_t) can be truncated. So this needs to be handled too.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: udf: Incorrect handling of timezone

[Jan Kara]

On Sun 19-01-20 13:49:44, Pali Rohár wrote:
> Hello! I looked at udf code which converts linux time to UDF time and I
> found out that conversion of timezone is incorrect.
> 
> Relevant code from udf_time_to_disk_stamp() function:
> 
> 	int16_t offset;
> 
> 	offset = -sys_tz.tz_minuteswest;
> 
> 	dest->typeAndTimezone = cpu_to_le16(0x1000 | (offset & 0x0FFF));
> 
> UDF 2.60 2.1.4.1 Uint16 TypeAndTimezone; says:
> 
>   For the following descriptions Type refers to the most significant 4 bits of this
>   field, and TimeZone refers to the least significant 12 bits of this field, which is
>   interpreted as a signed 12-bit number in two’s complement form.
> 
>   TimeZone ... If this field contains -2047 then the time zone has not been specified.
> 
> As offset is of signed 16bit integer, (offset & 0x0FFF) result always
> clears sign bit and therefore timezone is stored to UDF fs incorrectly.

I don't think the code is actually wrong. Two's complement has a nice
properly that changing type width can be done just by masking - as an
example -21 in s32 is 0xffffffeb, if you reduce to just 12-bits, you get
0xfeb which is still proper two's complement encoding of -21 for 12-bit wide
numbers.

> This needs to be fixed, sign bit from tz_minuteswest needs to be
> propagated to 12th bit in typeAndTimezone member.
> 
> Also tz_minuteswest is of int type, so conversion to int16_t (or more
> precisely int12_t) can be truncated. So this needs to be handled too.

tz_minuteswest is limited to +-15 hours (i.e., 900 minutes) so we shouldn't
need to handle truncating explicitely.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: udf: Incorrect handling of timezone

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On Monday 20 January 2020 12:31:16 Jan Kara wrote:
> On Sun 19-01-20 13:49:44, Pali Rohár wrote:
> > Hello! I looked at udf code which converts linux time to UDF time and I
> > found out that conversion of timezone is incorrect.
> > 
> > Relevant code from udf_time_to_disk_stamp() function:
> > 
> > 	int16_t offset;
> > 
> > 	offset = -sys_tz.tz_minuteswest;
> > 
> > 	dest->typeAndTimezone = cpu_to_le16(0x1000 | (offset & 0x0FFF));
> > 
> > UDF 2.60 2.1.4.1 Uint16 TypeAndTimezone; says:
> > 
> >   For the following descriptions Type refers to the most significant 4 bits of this
> >   field, and TimeZone refers to the least significant 12 bits of this field, which is
> >   interpreted as a signed 12-bit number in two’s complement form.
> > 
> >   TimeZone ... If this field contains -2047 then the time zone has not been specified.
> > 
> > As offset is of signed 16bit integer, (offset & 0x0FFF) result always
> > clears sign bit and therefore timezone is stored to UDF fs incorrectly.
> 
> I don't think the code is actually wrong. Two's complement has a nice
> properly that changing type width can be done just by masking - as an
> example -21 in s32 is 0xffffffeb, if you reduce to just 12-bits, you get
> 0xfeb which is still proper two's complement encoding of -21 for 12-bit wide
> numbers.

Ou right, so it is really OK. Thank you for clarification.

> > This needs to be fixed, sign bit from tz_minuteswest needs to be
> > propagated to 12th bit in typeAndTimezone member.
> > 
> > Also tz_minuteswest is of int type, so conversion to int16_t (or more
> > precisely int12_t) can be truncated. So this needs to be handled too.
> 
> tz_minuteswest is limited to +-15 hours (i.e., 900 minutes) so we shouldn't
> need to handle truncating explicitely.

Fine, then code is correct.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]