[ added TI DaVinci platform Maintainers to Cc: ]

Hi,

On 5/22/20 5:01 AM, Changming Liu wrote:

Hi Bartlomiej, Greetings, it's me again, I sent you a bug report yesterday, I hope that find you well.

This time I found that in /drivers/video/fbdev/da8xx-fb.c function lcd_cfg_vertical_sync, there might be an undefined result by left shifting.

More specifically, in function lcd_cfg_vertical_sync, line 437. back_porch is a signed integer which might come from user space. And it's logic AND with string literal 0xff. The result is then left shifted by 24 bits.

The problem is, since the logic AND produce a signed integer and the result of left shifting this signed integer (whose lowest 8 bits not cleared) by 24 bits is undefined when its 8th bit is 1. Similar patterns can be found in line 410 as well.

I wonder if this bug is worth fixing? This can help me understand linux and UB a lot.

Looking forward to you valuable response.

I assume that lcd_cfg_{horizontal,vertical}_sync() take parameters as signed integers (and not unsigned ones) in order to special case "0" value (I suppose that hardware expects all bits sets to represent the "0" value). Sekhar/Bartosz: could you verify this?

If the above is true to get rid of undefined behaviors in the code (BTW gcc seems to produce correct results currently, I don't know about clang) we should add type casts in proper places, i.e:

static void lcd_cfg_horizontal_sync(int back_porch, int pulse_width, int front_porch) { u32 reg;

reg = lcdc_read(LCD_RASTER_TIMING_0_REG) & 0x3ff; reg |= (((u32)(back_porch - 1) & 0xff) << 24) | (((u32)(front_porch - 1) & 0xff) << 16) | (((u32)(pulse_width - 1) & 0x3f) << 10); lcdc_write(reg, LCD_RASTER_TIMING_0_REG);

/* * LCDC Version 2 adds some extra bits that increase the allowable * size of the horizontal timing registers. * remember that the registers use 0 to represent 1 so all values * that get set into register need to be decremented by 1 */ if (lcd_revision == LCD_VERSION_2) { /* Mask off the bits we want to change */ reg = lcdc_read(LCD_RASTER_TIMING_2_REG) & ~0x780000ff; reg |= ((u32)(front_porch - 1) & 0x300) >> 8; reg |= ((u32)(back_porch - 1) & 0x300) >> 4; reg |= ((u32)(pulse_width - 1) & 0x3c0) << 21; lcdc_write(reg, LCD_RASTER_TIMING_2_REG); } }

static void lcd_cfg_vertical_sync(int back_porch, int pulse_width, int front_porch) { u32 reg;

reg = lcdc_read(LCD_RASTER_TIMING_1_REG) & 0x3ff; reg |= (((u32)back_porch & 0xff) << 24) | (((u32)front_porch & 0xff) << 16) | (((u32)(pulse_width - 1) & 0x3f) << 10); lcdc_write(reg, LCD_RASTER_TIMING_1_REG); }

Also it would be helpful to disallow negative values being passed from user-space in fb_ioctl().

Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics

Best regards, Changming Liu