Hi Andre,
On 1/12/19 11:45 PM, Andre Naujoks wrote:
I really don't know. That's why I'd be hesitant to restrict this. Maybe
limit it to something really out of the ordinary, like a year?
:-)
The intention was to send and monitor cyclic CAN frames within a range
of 5 to 5000ms. Even if you want to ping a satellite over CAN in the
deep space network ... I would like to introduce some kind of
restriction after all.
The question is whether e.g. low power use-cases would require some very
seldom pings to be monitored.
I am not sure that for example one hour would be out of the question for
some edge cases. Maybe someone wants to do a heartbeat for his/her
system with a very low priority. This would mean a TX_SETUP with a
timeout of an hour and a RX_SETUP with a timeout of a bit more.
If the system allow timeouts in those ranges, I think it should be
allowed. If someone wants to wait a year for a CAN frame, however
unlikely that might be, why not?
That's scary. IMO you would go for another technical approach if you
want to communicate over this period of time.
Anyway if it's ok for you I would limit the timer to 400 days to have a
least a limitation when sending the V2 patch after getting feedback from
Kyungtae Kim.
Thanks for stepping in!
Best,
Oliver
Andre.
On 1/12/19 11:30 PM, Oliver Hartkopp wrote:
Hi Andre,
just wondered whether it makes sense to limit this value for sending
cyclic messages or for detecting a timeout on reception.
4.294.967.295 seconds would be ~136 years - this makes no sense to me
and I would assume someone applied some (unintended?) stuff into the
timeval.
Don't you think?
Best,
Oliver
On 1/12/19 11:16 PM, Andre Naujoks wrote:
Hi.
The 15 minute limit seems arbitrary to me. I'd be surprised if an
(R|T)X_SETUP failed because of a timeout greater than this. Are there
any problems with allowing larger timeouts? If not, I do not see a
reason to restrict this.
Regards
Andre
On 1/12/19 10:57 PM, Oliver Hartkopp wrote:
Kyungtae Kim detected a potential integer overflow in
bcm_[rx|tx]_setup() when
the conversion into ktime multiplies the given value with
NSEC_PER_USEC (1000).
Reference: https://marc.info/?l=linux-can&m=154732118819828&w=2
Add a check for the given tv_usec, so that the value stays below one
second.
Additionally limit the tv_sec value to a reasonable value for CAN
related
use-cases of 15 minutes.
Reported-by: Kyungtae Kim <kt0755@xxxxxxxxx>
Tested-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Cc: linux-stable <stable@xxxxxxxxxxxxxxx> # >= 2.6.26
---
net/can/bcm.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 0af8f0db892a..ff3799be077b 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -67,6 +67,9 @@
*/
#define MAX_NFRAMES 256
+/* limit timers to 15 minutes for sending/timeouts */
+#define BCM_TIMER_SEC_MAX (15*60)
+
/* use of last_frames[index].flags */
#define RX_RECV 0x40 /* received data for this element */
#define RX_THR 0x80 /* element not been sent due to throttle
feature */
@@ -140,6 +143,18 @@ static inline ktime_t
bcm_timeval_to_ktime(struct bcm_timeval tv)
return ktime_set(tv.tv_sec, tv.tv_usec * NSEC_PER_USEC);
}
+/* check limitations for timeval provided by user */
+static int bcm_is_invalid_tv(struct bcm_msg_head *msg_head)
+{
+ if ((msg_head->ival1.tv_sec > BCM_TIMER_SEC_MAX) ||
+ (msg_head->ival1.tv_usec >= USEC_PER_SEC) ||
+ (msg_head->ival2.tv_sec > BCM_TIMER_SEC_MAX) ||
+ (msg_head->ival2.tv_usec >= USEC_PER_SEC))
+ return 1;
+
+ return 0;
+}
+
#define CFSIZ(flags) ((flags & CAN_FD_FRAME) ? CANFD_MTU : CAN_MTU)
#define OPSIZ sizeof(struct bcm_op)
#define MHSIZ sizeof(struct bcm_msg_head)
@@ -873,6 +888,10 @@ static int bcm_tx_setup(struct bcm_msg_head
*msg_head, struct msghdr *msg,
if (msg_head->nframes < 1 || msg_head->nframes > MAX_NFRAMES)
return -EINVAL;
+ /* check timeval limitations */
+ if ((msg_head->flags & SETTIMER) && bcm_is_invalid_tv(msg_head))
+ return -EINVAL;
+
/* check the given can_id */
op = bcm_find_op(&bo->tx_ops, msg_head, ifindex);
if (op) {
@@ -1053,6 +1072,10 @@ static int bcm_rx_setup(struct bcm_msg_head
*msg_head, struct msghdr *msg,
(!(msg_head->can_id & CAN_RTR_FLAG))))
return -EINVAL;
+ /* check timeval limitations */
+ if ((msg_head->flags & SETTIMER) && bcm_is_invalid_tv(msg_head))
+ return -EINVAL;
+
/* check the given can_id */
op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
if (op) {
