You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2023/01/09 06:48:44 UTC
[GitHub] [nuttx] csanchezdll opened a new pull request, #8058: s32k1xx: avoid buffer overflow when CAN time is used for non-FD CAN.
csanchezdll opened a new pull request, #8058:
URL: https://github.com/apache/nuttx/pull/8058
s32k1xx: fix initialization of MAXMB field in MCR.
## Summary
This includes two fixes for s32k1xx CAN driver.
First, MSG_DATA was not included in TX/RX buffer frames for non-FD builds of the driver. However, both CONFIG_NET_CAN_RAW_TX_DEADLINE and CONFIG_NET_TIMESTAMP can be used with non-FD CAN, and doing do would cause a buffer overflow as code expects extra space for MSG_DATA to be present in those cases.
Second, MAXMB field of MCR has a reset value of 0xF. Current code just OR's the desired value into this register without cleaning it first. For the settings in the file (5 RX MBs and 2 TX MBs) this means a 7 should end up there, but without my patch, the reset 0xF is not correctly overwritten meaning the HW device tries to use unconfigured mailboxes when the expeted & configured ones are full.
## Impact
Buffer overflow is serious and will break any code using time features. In my case, I am using CONFIG_NET_TIMESTAMP and SO_TIMESTAMP in my CAN sockets and I get a buffer overflow.
The mailbox initialization will cause data loss as received CAN frames are placed on those unexpected mailboxes and RX code never checks them. On low-traffic scenarios this will be very unlikely as the first 5 RX mailboxes will be empty most of the time, not causing the "unconfigured" ones to be used.
## Testing
I have observed both issues on my custom HW.
To reproduce buffer overflow: just create a socket CAN and ioctl(...SO_TIMESTAMP...) to enable timestamping.
To observe MAXMB problem: step into s32k1xx_reset using gdb and check the value of MCR after it is written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] csanchezdll commented on a diff in pull request #8058: s32k1xx: avoid buffer overflow when CAN time is used for non-FD CAN.
Posted by GitBox <gi...@apache.org>.
csanchezdll commented on code in PR #8058:
URL: https://github.com/apache/nuttx/pull/8058#discussion_r1064650963
##########
arch/arm/src/s32k1xx/s32k1xx_flexcan.c:
##########
@@ -341,8 +341,8 @@ static struct s32k1xx_driver_s g_flexcan2;
static uint8_t g_tx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
static uint8_t g_rx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
#else
-static uint8_t g_tx_pool[sizeof(struct can_frame)*POOL_SIZE];
-static uint8_t g_rx_pool[sizeof(struct can_frame)*POOL_SIZE];
+static uint8_t g_tx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
+static uint8_t g_rx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
Review Comment:
Some drivers reserve it always, some reserve it conditionally. Actually, stm32 driver reserves that space only for CONFIG_NET_CAN_RAW_TX_DEADLINE, which causes a similar buffer overflow when CONFIG_NET_TIMESTAMP is used. Another colleague of mine was going to submit that patch for stm32, unsure about the current status. I prefer keeping things decoupled. But I can add the conditional definition of MSG_DATA for s32k1xx , if you prefer to include it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on a diff in pull request #8058: s32k1xx: avoid buffer overflow when CAN time is used for non-FD CAN.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #8058:
URL: https://github.com/apache/nuttx/pull/8058#discussion_r1064628110
##########
arch/arm/src/s32k1xx/s32k1xx_flexcan.c:
##########
@@ -341,8 +341,8 @@ static struct s32k1xx_driver_s g_flexcan2;
static uint8_t g_tx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
static uint8_t g_rx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
#else
-static uint8_t g_tx_pool[sizeof(struct can_frame)*POOL_SIZE];
-static uint8_t g_rx_pool[sizeof(struct can_frame)*POOL_SIZE];
+static uint8_t g_tx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
+static uint8_t g_rx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
Review Comment:
Should we
```
#if defined(CONFIG_NET_CAN_RAW_TX_DEADLINE) || defined(CONFIG_NET_TIMESTAMP)
# define MSG_DATA sizeof(struct timeval)
#else
# define MSG_DATA 0
#endif
```
instead of reserving more memory always?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] csanchezdll commented on a diff in pull request #8058: s32k1xx: avoid buffer overflow when CAN time is used for non-FD CAN.
Posted by GitBox <gi...@apache.org>.
csanchezdll commented on code in PR #8058:
URL: https://github.com/apache/nuttx/pull/8058#discussion_r1064650963
##########
arch/arm/src/s32k1xx/s32k1xx_flexcan.c:
##########
@@ -341,8 +341,8 @@ static struct s32k1xx_driver_s g_flexcan2;
static uint8_t g_tx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
static uint8_t g_rx_pool[(sizeof(struct canfd_frame)+MSG_DATA)*POOL_SIZE];
#else
-static uint8_t g_tx_pool[sizeof(struct can_frame)*POOL_SIZE];
-static uint8_t g_rx_pool[sizeof(struct can_frame)*POOL_SIZE];
+static uint8_t g_tx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
+static uint8_t g_rx_pool[(sizeof(struct can_frame)+MSG_DATA)*POOL_SIZE];
Review Comment:
Some drivers reserve it always, some reserve it conditionally. Actually, stm32 driver reserves that space only for CONFIG_NET_CAN_RAW_TX_DEADLINE, which causes a similar buffer overflow when CONFIG_NET_TIMESTAMP is used. Another colleague of mine was going to submit that patch for stm32, unsure about the current status. I prefer keeping things decoupled. But I can add the conditional definition of MSG_DATA for s32k1xx , if you prefer to include it here.
Edit: I see that other change is already merged: https://github.com/apache/nuttx/pull/7194
I am thus adding the same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko merged pull request #8058: s32k1xx: avoid buffer overflow when CAN time is used for non-FD CAN.
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #8058:
URL: https://github.com/apache/nuttx/pull/8058
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org