From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Naujoks Subject: Re: [PATCH] can: bcm: check timer values before ktime conversion Date: Sat, 12 Jan 2019 23:45:29 +0100 Message-ID: References: <20190112215726.2622-1-socketcan@hartkopp.net> <01976e61-2ba1-09bf-94a1-b0e879c9a07b@gmail.com> <1db63586-5ce6-7ec5-f076-ec8d57dffe7d@hartkopp.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: linux-can@vger.kernel.org, lifeasageek@gmail.com, threeearcat@gmail.com, syzkaller@googlegroups.com, Kyungtae Kim , linux-stable To: Oliver Hartkopp , davem@davemloft.net, netdev@vger.kernel.org Return-path: In-Reply-To: <1db63586-5ce6-7ec5-f076-ec8d57dffe7d@hartkopp.net> Content-Language: en-US Sender: stable-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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? 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? 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 >>> Tested-by: Oliver Hartkopp >>> Signed-off-by: Oliver Hartkopp >>> Cc: linux-stable # >= 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) { >>> >> From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2AC5BC43612 for ; Sat, 12 Jan 2019 22:45:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id ED16D2086C for ; Sat, 12 Jan 2019 22:45:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="fYKqE/rj" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726568AbfALWpe (ORCPT ); Sat, 12 Jan 2019 17:45:34 -0500 Received: from mail-wr1-f65.google.com ([209.85.221.65]:33454 "EHLO mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726525AbfALWpd (ORCPT ); Sat, 12 Jan 2019 17:45:33 -0500 Received: by mail-wr1-f65.google.com with SMTP id c14so18985009wrr.0; Sat, 12 Jan 2019 14:45:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=lGQ7XtsSwxFYOfy45iWNekskwbKY2rHfI55KcLUgyeI=; b=fYKqE/rjTyVbqad5PvdXsjT16/FIXrvwqlynjiIoDsWFmW7190ZiJLbrLz0EKdtuBC lYPKR4F8HRPYN+bNB/RcF0c3HdUkRLWrW0mnhssqNtLaTvQbeEZL27r5FCuuzGeEayNN NCA1jWtCIZpr1hmBBXd43jMGEdMUUSPeCPeMT0vEOtUuPK/j+P86UOi8wNlvP15gI1c8 eXtufzSriu/EgDsbk9UPSy5OGQQyFLYMnEF8VWiZzly7NNExd7wldZe/04UUW+XeNNiM zXjCs3KycvhAvot/PTot067KdjF+8VEpJh5e2UqVP3QnlF5ng4a1BS3XENSHtOM5iIO8 F/iQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=lGQ7XtsSwxFYOfy45iWNekskwbKY2rHfI55KcLUgyeI=; b=IImdnDM8z1Ep/M8x3ZzDUq9fTE97pdDIOMETLIM+qwZIPBZK3HtuztaZ9Vqng1cSBf OJcBem2l5shJ2iE2s+1rvMsedyrUtU90eOMSnzeHJ6dB7a7bewzrxGQM8CD5S+5u12Fw 7f1CYG9g3XI/ralJVi8MkwleoNCGzEq0J/3dfqZ45uHJFF+qhI2SFqfoXgDZjTbpjHxX uzlkKSq8p4R1BhhXcUz4IMKBWZj3Y8kS2GItLR16+jtGCAbiAWJ0zpDpTes3L1WRzF5A m5KPVfjeUGU2MMJrkbyjhdST+2sJvfVO3cKrA33BKUlTtZmSVIDf/uoZ1AzvemxlADxX otkw== X-Gm-Message-State: AJcUukfQ2m4kY/tJCgn5wGCa8/fwLPxyw714SJKld2YtHCi+UvOUDQ3b oip2OuJHZlYuBJJWH7VdJSX/2+eyLIs= X-Google-Smtp-Source: ALg8bN6woIfrpYG5XoEtDMB6OudZUUajdyM4s/rd36BpD5u51mDQK2DUGDTbXC+50ZAAVGvN+Gxusw== X-Received: by 2002:adf:de91:: with SMTP id w17mr19843316wrl.320.1547333130843; Sat, 12 Jan 2019 14:45:30 -0800 (PST) Received: from ?IPv6:2003:ee:f1a:bb00:cccb:495f:8763:e3b1? (p200300EE0F1ABB00CCCB495F8763E3B1.dip0.t-ipconnect.de. [2003:ee:f1a:bb00:cccb:495f:8763:e3b1]) by smtp.googlemail.com with ESMTPSA id o82sm21038842wmo.29.2019.01.12.14.45.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 12 Jan 2019 14:45:30 -0800 (PST) Subject: Re: [PATCH] can: bcm: check timer values before ktime conversion To: Oliver Hartkopp , davem@davemloft.net, netdev@vger.kernel.org Cc: linux-can@vger.kernel.org, lifeasageek@gmail.com, threeearcat@gmail.com, syzkaller@googlegroups.com, Kyungtae Kim , linux-stable References: <20190112215726.2622-1-socketcan@hartkopp.net> <01976e61-2ba1-09bf-94a1-b0e879c9a07b@gmail.com> <1db63586-5ce6-7ec5-f076-ec8d57dffe7d@hartkopp.net> From: Andre Naujoks Message-ID: Date: Sat, 12 Jan 2019 23:45:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <1db63586-5ce6-7ec5-f076-ec8d57dffe7d@hartkopp.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Message-ID: <20190112224529.pX9OsB1huoXhwRJuf-SZXupzgQLLwinvanMlfXmnURE@z> 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? 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? 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 >>> Tested-by: Oliver Hartkopp >>> Signed-off-by: Oliver Hartkopp >>> Cc: linux-stable # >= 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) { >>> >>