lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Mar 2022 21:49:27 +0900 From: Vincent MAILHOL <mailhol.vincent@...adoo.fr> To: Marc Kleine-Budde <mkl@...gutronix.de> Cc: kernel test robot <yujie.liu@...el.com>, kbuild-all@...ts.01.org, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, Arunachalam Santhanam <Arunachalam.Santhanam@...bosch.com> Subject: Re: drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar] On Wed. 2 Mar 2022 at 19:32, Marc Kleine-Budde <mkl@...gutronix.de> wrote: > On 02.03.2022 17:47:08, kernel test robot wrote: > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > > head: 575115360652e9920cc56a028a286ebe9bf82694 > > commit: c664e2137a27680922d8aeb64fb10313416b254f can: etas_es58x: add support for the ETAS ES58X_FD CAN USB interfaces > > date: 11 months ago > > compiler: powerpc64-linux-gcc (GCC) 11.2.0 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <yujie.liu@...el.com> Do we still need the tag for false positives (c.f. below)? I am fine to add it, but maybe this will mess with your statistics? > > > > cppcheck possible warnings: (new ones prefixed by >>, may not be real problems) Indeed, not a real problem. My wild guess is that cppcheck does not recognize __stringify() as a pre-procesor macro. That would not be the first time a static analyzer would fail on it. checkpatch had the same issue which I fixed in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b844345fc2a9c46f8bb8cdb7408c766dfcdd83d Or maybe cppcheck does not try to expand the macro and directly yield a warning? Overall, I think that GCC/Clang already does a good job at finding usage of uninitialized variable. If cppcheck is less mature in this aspect, I suggest to deactivate this particular cppcheck rule and leave this job to GCC/Clang's -Wuninitialized. > > In file included from drivers/net/can/usb/etas_es58x/es58x_fd.c: > > >> drivers/net/can/usb/etas_es58x/es58x_fd.c:174:8: warning: Uninitialized variable: rx_event_msg [uninitvar] > > ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len); > > ^ > > > > vim +174 drivers/net/can/usb/etas_es58x/es58x_fd.c > > > > c664e2137a2768 Vincent Mailhol 2021-04-10 165 > > c664e2137a2768 Vincent Mailhol 2021-04-10 166 static int es58x_fd_rx_event_msg(struct net_device *netdev, > > c664e2137a2768 Vincent Mailhol 2021-04-10 167 const struct es58x_fd_urb_cmd *es58x_fd_urb_cmd) > > c664e2137a2768 Vincent Mailhol 2021-04-10 168 { > > c664e2137a2768 Vincent Mailhol 2021-04-10 169 struct es58x_device *es58x_dev = es58x_priv(netdev)->es58x_dev; > > c664e2137a2768 Vincent Mailhol 2021-04-10 170 u16 msg_len = get_unaligned_le16(&es58x_fd_urb_cmd->msg_len); > > c664e2137a2768 Vincent Mailhol 2021-04-10 @171 const struct es58x_fd_rx_event_msg *rx_event_msg; > > c664e2137a2768 Vincent Mailhol 2021-04-10 172 int ret; > > c664e2137a2768 Vincent Mailhol 2021-04-10 173 > > c664e2137a2768 Vincent Mailhol 2021-04-10 @174 ret = es58x_check_msg_len(es58x_dev->dev, *rx_event_msg, msg_len); > > c664e2137a2768 Vincent Mailhol 2021-04-10 175 if (ret) > > c664e2137a2768 Vincent Mailhol 2021-04-10 176 return ret; > > c664e2137a2768 Vincent Mailhol 2021-04-10 177 > > c664e2137a2768 Vincent Mailhol 2021-04-10 178 rx_event_msg = &es58x_fd_urb_cmd->rx_event_msg; > > c664e2137a2768 Vincent Mailhol 2021-04-10 179 > > c664e2137a2768 Vincent Mailhol 2021-04-10 180 return es58x_rx_err_msg(netdev, rx_event_msg->error_code, > > c664e2137a2768 Vincent Mailhol 2021-04-10 181 rx_event_msg->event_code, > > c664e2137a2768 Vincent Mailhol 2021-04-10 182 get_unaligned_le64(&rx_event_msg->timestamp)); > > c664e2137a2768 Vincent Mailhol 2021-04-10 183 } > > c664e2137a2768 Vincent Mailhol 2021-04-10 184 > > Thanks for the report. > > This looks like a false positive to me, as es58x_check_msg_len() is not > a function, but a macro: > > | #define es58x_check_msg_len(dev, msg, actual_len) \ > | __es58x_check_msg_len(dev, __stringify(msg), \ > | actual_len, sizeof(msg)) > > __es58x_check_msg_len() don't use "rx_event_msg" directly, but only a > string representation of it and a "sizeof()". Ack. > I think it's possible to assign rx_event_msg before the > es58x_check_msg_len(). Yes, I will do so. Even if this is a false positive, this pattern can be misleading. e.g. during a code review, this does indeed look incorrect at first glance. Also, doing such change would be consistent with was is done in other functions: https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L210 This not being a bug fix, is it fine to send it to net-next? Or do you see a need to backport this? > I think (hope?) the compiler will not optimize > anything away. :) With a function call and a return statement, the compiler would need to be severely defective to try to optimize this away :) > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists