[PATCH] isofs: fix undefined behavior in iso_date()

[PATCH] isofs: fix undefined behavior in iso_date()

[Linke Li]

From: Linke Li <lilinke99@gmail.com>

Fix undefined behavior in the code by properly handling the left shift operaion.
Instead of left-shifting a negative value, explicitly cast -1 to an unsigned int
before the shift. This ensures well defined behavior and resolves any potential
issues.

Signed-off-by: Linke Li <lilinke99@gmail.com>
---
 fs/isofs/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/isofs/util.c b/fs/isofs/util.c
index e88dba721661..4c902901401a 100644
--- a/fs/isofs/util.c
+++ b/fs/isofs/util.c
@@ -37,7 +37,7 @@ int iso_date(u8 *p, int flag)
 
 		/* sign extend */
 		if (tz & 0x80)
-			tz |= (-1 << 8);
+			tz |= ((unsigned int)-1 << 8);
 		
 		/* 
 		 * The timezone offset is unreliable on some disks,
-- 
2.25.1

Re: [PATCH] isofs: fix undefined behavior in iso_date()

[Matthew Wilcox]

On Sun, Jul 09, 2023 at 02:42:55PM +0800, Linke Li wrote:
> From: Linke Li <lilinke99@gmail.com>
> 
> Fix undefined behavior in the code by properly handling the left shift operaion.
> Instead of left-shifting a negative value, explicitly cast -1 to an unsigned int
> before the shift. This ensures well defined behavior and resolves any potential
> issues.

This certainly fixes the problem, but wouldn't it be easier to get the
compiler to do the work for us?

#include <stdio.h>

int f(unsigned char *p)
{
	return (signed char)p[0];
}

int main(void)
{
	unsigned char x = 0xa5;

	printf("%d\n", f(&x));

	return 0;
}

prints -91.

Re: [PATCH] isofs: fix undefined behavior in iso_date()

[linke li]

Dear Markus,

Thank you for your valuable feedback, I apologize for my typo in the
description.

> How do you think about to add the tag “Fixes”?

I agree with that.

Re: [PATCH] isofs: fix undefined behavior in iso_date()

[Dan Carpenter]

It looks like maybe there is an issue with "year" as well.

fs/isofs/util.c
    19  int iso_date(u8 *p, int flag)
    20  {
    21          int year, month, day, hour, minute, second, tz;
    22          int crtime;
    23
    24          year = p[0];
                       ^^^^^
year is 0-255.

    25          month = p[1];
    26          day = p[2];
    27          hour = p[3];
    28          minute = p[4];
    29          second = p[5];
    30          if (flag == 0) tz = p[6]; /* High sierra has no time zone */
    31          else tz = 0;
    32          
    33          if (year < 0) {
                    ^^^^^^^^
But this checks year for < 0 which is impossible.  Should it be:

	year = (signed char)p[0];?

    34                  crtime = 0;
    35          } else {
    36                  crtime = mktime64(year+1900, month, day, hour, minute, second);
    37
    38                  /* sign extend */
    39                  if (tz & 0x80)
    40                          tz |= (-1 << 8);
    41                  
    42                  /*

regards,
dan carpenter

Re: [PATCH] isofs: fix undefined behavior in iso_date()

[linke li]

Thanks for your reply.
> This certainly fixes the problem, but wouldn't it be easier to get the
> compiler to do the work for us?
 I don't know which solution is better, but it does avoid this problem.
Like
    tz = (int)(signed char)p[6];

RE: [PATCH] isofs: fix undefined behavior in iso_date()

[David Laight]

From: Dan Carpenter
> Sent: 10 July 2023 10:57
> 
> It looks like maybe there is an issue with "year" as well.
> 
> fs/isofs/util.c
>     19  int iso_date(u8 *p, int flag)
>     20  {
>     21          int year, month, day, hour, minute, second, tz;
>     22          int crtime;
>     23
>     24          year = p[0];
>                        ^^^^^
> year is 0-255.
....
>     32
>     33          if (year < 0) {
>                     ^^^^^^^^
> But this checks year for < 0 which is impossible.  Should it be:
> 
> 	year = (signed char)p[0];?

Or not?

What happens in 2027 ?
I bet the value has to be treated an unsigned.

> 
>     34                  crtime = 0;
>     35          } else {
>     36                  crtime = mktime64(year+1900, month, day, hour, minute, second);
>     37
>     38                  /* sign extend */
>     39                  if (tz & 0x80)
>     40                          tz |= (-1 << 8);

Just change the definition of tz from 'int' to 's8'
and it will all happen 'by magic'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] isofs: fix undefined behavior in iso_date()

[Dan Carpenter]

On Thu, Jul 13, 2023 at 02:11:02PM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 10 July 2023 10:57
> > 
> > It looks like maybe there is an issue with "year" as well.
> > 
> > fs/isofs/util.c
> >     19  int iso_date(u8 *p, int flag)
> >     20  {
> >     21          int year, month, day, hour, minute, second, tz;
> >     22          int crtime;
> >     23
> >     24          year = p[0];
> >                        ^^^^^
> > year is 0-255.
> ....
> >     32
> >     33          if (year < 0) {
> >                     ^^^^^^^^
> > But this checks year for < 0 which is impossible.  Should it be:
> > 
> > 	year = (signed char)p[0];?
> 
> Or not?
> 
> What happens in 2027 ?
> I bet the value has to be treated an unsigned.

Yeah.  Good point.  We could delete that if statement and pull the whole
function in a tab.

regards,
dan carpenter