arch/arm/src/at32/at32_can: Division by Zero in CAN Bit Timing Commands

[catalinv-ncc]

arch/arm/src/at32/at32_can: Division by Zero in CAN Bit Timing Commands

An unchecked integer assignment in the CAN driver may result in a division-by-zero which would result in a kernel crash (denial of service).

Tested locally, builds fine.

Impact

An unchecked integer assignment in the CAN driver may result in a division-by-zero which would result in a kernel crash (denial of service).

Description

The CAN driver can_ioctl function, shown below (drivers/can/can.c), receives commands from user processes. Its received arguments cmd and content of arg are under attacker's control. In the CAN driver, some ioctl commands are hardware specific and are processed by calling dev_ioctl(). Subsequently, depending on the hardware (STM32 or AT32), this function calls fdcan_ioctl or at32can_ioctl, as shown in the snippets that follow.

static int can_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
{
  FAR struct inode        *inode  = filep->f_inode;
  FAR struct can_dev_s    *dev    = inode->i_private;
  FAR struct can_reader_s *reader = filep->f_priv;
...
  flags = enter_critical_section();

  /* Handle built-in ioctl commands */
  switch (cmd)
    {
...
      /* Not a "built-in" ioctl command.. perhaps it is unique to this
       * lower-half, device driver. */
      default:
        {
          ret = dev_ioctl(dev, cmd, arg);
        }
        break;
    }

  leave_critical_section(flags);
  return ret;
}

There are a few instances where the user can trigger a kernel crash, caused by a division by zero on CANIOC_SET_BITTIMING command. On STM32 platforms (arch/arm/src/stm32/stm32_fdcan.c), while there is a DEBUGASSERT assertion for bt->bt_baud, the check does not cover value 0.

static const struct can_ops_s g_fdcanops =
{
...
  .co_ioctl         = fdcan_ioctl,
...
};

static int fdcan_ioctl(struct can_dev_s *dev, int cmd, unsigned long arg)
{
...
  switch (cmd)
    {
...
      case CANIOC_SET_BITTIMING:
        {
          const struct canioc_bittiming_s *bt = (const struct canioc_bittiming_s *)arg;
          uint32_t nbrp;
          uint32_t ntseg1;
          uint32_t ntseg2;
          uint32_t nsjw;
          uint32_t ie;
          uint8_t state;

          DEBUGASSERT(bt != NULL);
          DEBUGASSERT(bt->bt_baud < STM32_FDCANCLK_FREQUENCY); // <- not valid
          DEBUGASSERT(bt->bt_sjw > 0 && bt->bt_sjw <= 16);
          DEBUGASSERT(bt->bt_tseg1 > 1 && bt->bt_tseg1 <= 64);
          DEBUGASSERT(bt->bt_tseg2 > 0 && bt->bt_tseg2 <= 16);

          /* Extract bit timing data */
          ntseg1 = bt->bt_tseg1 - 1;
          ntseg2 = bt->bt_tseg2 - 1;
          nsjw   = bt->bt_sjw   - 1;

          nbrp = (uint32_t)
            (  ((float) STM32_FDCANCLK_FREQUENCY /
               ((float)(ntseg1 + ntseg2 + 3) * (float)bt->bt_baud)) - 1 ); // <- div by 0

Similarly, another division by zero was found in Artery Technology AT32 (arch/arm/src/stm32/stm32_can.c) driver:

static const struct can_ops_s g_canops =
{
...
  .co_ioctl         = at32can_ioctl,
...
};

static int at32can_ioctl(struct can_dev_s *dev, int cmd, unsigned long arg)
{
...
  /* Handle the command */
  switch (cmd)
    {
...
      case CANIOC_SET_BITTIMING:
        {
          const struct canioc_bittiming_s *bt = (const struct canioc_bittiming_s *)arg;
...
          uint32_t tmp;
          uint32_t regval;

          DEBUGASSERT(bt != NULL);
          DEBUGASSERT(bt->bt_baud < AT32_PCLK1_FREQUENCY); // <- not valid
          DEBUGASSERT(bt->bt_sjw > 0 && bt->bt_sjw <= 4);
          DEBUGASSERT(bt->bt_tseg1 > 0 && bt->bt_tseg1 <= 16);
          DEBUGASSERT(bt->bt_tseg2 > 0 && bt->bt_tseg2 <=  8);

          regval = at32can_getreg(priv, AT32_CAN_BTMG_OFFSET);

          /* Extract bit timing data tmp is in clocks per bit time */
          tmp = AT32_PCLK1_FREQUENCY / bt->bt_baud; // <- div by 0

Instances found are listed in the Location section below. They are not shown in detail to reduce the length of the issue.

Recommendation

Ensure the attacker-controlled data is properly validated before use, to stop division by zero situations. For instance:

DEBUGASSERT(bt->bt_baud > 0 && bt->bt_baud < AT32_PCLK1_FREQUENCY);

Location

• arch/arm/src/stm32/stm32_fdcan.c
• arch/arm/src/stm32/stm32_can.c
• arch/arm/src/sama5/sam_mcan.c
• arch/arm/src/at32/at32_can.c
• arch/arm/src/samv7/sam_mcan.c
• arch/arm/src/stm32f0l0g0/stm32_fdcan.c
• arch/arm/src/stm32f7/stm32_can.c
• arch/arm/src/stm32h5/stm32_fdcan.c
• arch/arm/src/stm32l4/stm32l4_can.c
• arch/arm/src/tiva/common/tiva_can.c
• drivers/can/mcp2515.c

[catalinv-ncc]

Should this be:

DEBUGASSERT(bt->bt_baud > 0 && SYSCLK_FREQUENCY < bt->bt_baud);