| Message ID | 20190617165836.4673-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: add sanity check to device_property_read_u32_array call | expand |
From: Colin King <colin.king@canonical.com> Date: Mon, 17 Jun 2019 17:58:36 +0100 > From: Colin Ian King <colin.king@canonical.com> > > Currently the call to device_property_read_u32_array is not error checked > leading to potential garbage values in the delays array that are then used > in msleep delays. Add a sanity check to the property fetching. > > Addresses-Coverity: ("Uninitialized scalar variable") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Applied to net-next, thanks Colin. Please make the destination tree explicit in the future by putting something like "[PATCH net-next]" in the Subject line. Thank you.
Hi Colin, > Currently the call to device_property_read_u32_array is not error checked > leading to potential garbage values in the delays array that are then used > in msleep delays. Add a sanity check to the property fetching. > > Addresses-Coverity: ("Uninitialized scalar variable") > Signed-off-by: Colin Ian King <colin.king@canonical.com> I have also sent a patch [0] to fix initialize the array. can you please look at my patch so we can work out which one to use? my concern is that the "snps,reset-delays-us" property is optional, the current dt-bindings documentation states that it's a required property. in reality it isn't, there are boards (two examples are mentioned in my patch: [0]) without it. so I believe that the resulting behavior has to be: 1. don't delay if this property is missing (instead of delaying for <garbage value> ms) 2. don't error out if this property is missing your patch covers #1, can you please check whether #2 is also covered? I tested case #2 when submitting my patch and it worked fine (even though I could not reproduce the garbage values which are being read on some boards) Thank you! Martin [0] https://lkml.org/lkml/2019/4/19/638
On 19/06/2019 06:13, Martin Blumenstingl wrote: > Hi Colin, > >> Currently the call to device_property_read_u32_array is not error checked >> leading to potential garbage values in the delays array that are then used >> in msleep delays. Add a sanity check to the property fetching. >> >> Addresses-Coverity: ("Uninitialized scalar variable") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > I have also sent a patch [0] to fix initialize the array. > can you please look at my patch so we can work out which one to use? > > my concern is that the "snps,reset-delays-us" property is optional, > the current dt-bindings documentation states that it's a required > property. in reality it isn't, there are boards (two examples are > mentioned in my patch: [0]) without it. > > so I believe that the resulting behavior has to be: > 1. don't delay if this property is missing (instead of delaying for > <garbage value> ms) > 2. don't error out if this property is missing > > your patch covers #1, can you please check whether #2 is also covered? > I tested case #2 when submitting my patch and it worked fine (even > though I could not reproduce the garbage values which are being read > on some boards) > > > Thank you! > Martin > > > [0] https://lkml.org/lkml/2019/4/19/638 > Is that the correct link? Colin
Hi Colin, On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: > > On 19/06/2019 06:13, Martin Blumenstingl wrote: > > Hi Colin, > > > >> Currently the call to device_property_read_u32_array is not error checked > >> leading to potential garbage values in the delays array that are then used > >> in msleep delays. Add a sanity check to the property fetching. > >> > >> Addresses-Coverity: ("Uninitialized scalar variable") > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > I have also sent a patch [0] to fix initialize the array. > > can you please look at my patch so we can work out which one to use? > > > > my concern is that the "snps,reset-delays-us" property is optional, > > the current dt-bindings documentation states that it's a required > > property. in reality it isn't, there are boards (two examples are > > mentioned in my patch: [0]) without it. > > > > so I believe that the resulting behavior has to be: > > 1. don't delay if this property is missing (instead of delaying for > > <garbage value> ms) > > 2. don't error out if this property is missing > > > > your patch covers #1, can you please check whether #2 is also covered? > > I tested case #2 when submitting my patch and it worked fine (even > > though I could not reproduce the garbage values which are being read > > on some boards) > > > > > > Thank you! > > Martin > > > > > > [0] https://lkml.org/lkml/2019/4/19/638 > > > Is that the correct link? sorry, that is a totally unrelated link the correct link is: https://patchwork.ozlabs.org/patch/1118313/
Hi Colin, On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Colin, > > On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: > > > > On 19/06/2019 06:13, Martin Blumenstingl wrote: > > > Hi Colin, > > > > > >> Currently the call to device_property_read_u32_array is not error checked > > >> leading to potential garbage values in the delays array that are then used > > >> in msleep delays. Add a sanity check to the property fetching. > > >> > > >> Addresses-Coverity: ("Uninitialized scalar variable") > > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > I have also sent a patch [0] to fix initialize the array. > > > can you please look at my patch so we can work out which one to use? > > > > > > my concern is that the "snps,reset-delays-us" property is optional, > > > the current dt-bindings documentation states that it's a required > > > property. in reality it isn't, there are boards (two examples are > > > mentioned in my patch: [0]) without it. > > > > > > so I believe that the resulting behavior has to be: > > > 1. don't delay if this property is missing (instead of delaying for > > > <garbage value> ms) > > > 2. don't error out if this property is missing > > > > > > your patch covers #1, can you please check whether #2 is also covered? > > > I tested case #2 when submitting my patch and it worked fine (even > > > though I could not reproduce the garbage values which are being read > > > on some boards) in the meantime I have tested your patch. when I don't set the "snps,reset-delays-us" property then I get the following error: invalid property snps,reset-delays-us my patch has landed in the meantime: [0] how should we proceed with your patch? Martin [0] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c?id=84ce4d0f9f55b4f4ca4d4edcbb54a23d9dad1aae
On 25/06/2019 05:44, Martin Blumenstingl wrote: > Hi Colin, > > On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> >> Hi Colin, >> >> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: >>> >>> On 19/06/2019 06:13, Martin Blumenstingl wrote: >>>> Hi Colin, >>>> >>>>> Currently the call to device_property_read_u32_array is not error checked >>>>> leading to potential garbage values in the delays array that are then used >>>>> in msleep delays. Add a sanity check to the property fetching. >>>>> >>>>> Addresses-Coverity: ("Uninitialized scalar variable") >>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>> I have also sent a patch [0] to fix initialize the array. >>>> can you please look at my patch so we can work out which one to use? >>>> >>>> my concern is that the "snps,reset-delays-us" property is optional, >>>> the current dt-bindings documentation states that it's a required >>>> property. in reality it isn't, there are boards (two examples are >>>> mentioned in my patch: [0]) without it. >>>> >>>> so I believe that the resulting behavior has to be: >>>> 1. don't delay if this property is missing (instead of delaying for >>>> <garbage value> ms) >>>> 2. don't error out if this property is missing >>>> >>>> your patch covers #1, can you please check whether #2 is also covered? >>>> I tested case #2 when submitting my patch and it worked fine (even >>>> though I could not reproduce the garbage values which are being read >>>> on some boards) > in the meantime I have tested your patch. > when I don't set the "snps,reset-delays-us" property then I get the > following error: > invalid property snps,reset-delays-us > > my patch has landed in the meantime: [0] > how should we proceed with your patch? I'm out of the office today. I'll get back to you on this tomorrow. Colin > > > Martin > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c?id=84ce4d0f9f55b4f4ca4d4edcbb54a23d9dad1aae >
On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote: > > On 25/06/2019 05:44, Martin Blumenstingl wrote: > > Hi Colin, > > > > On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > >> > >> Hi Colin, > >> > >> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: > >>> > >>> On 19/06/2019 06:13, Martin Blumenstingl wrote: > >>>> Hi Colin, > >>>> > >>>>> Currently the call to device_property_read_u32_array is not error checked > >>>>> leading to potential garbage values in the delays array that are then used > >>>>> in msleep delays. Add a sanity check to the property fetching. > >>>>> > >>>>> Addresses-Coverity: ("Uninitialized scalar variable") > >>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >>>> I have also sent a patch [0] to fix initialize the array. > >>>> can you please look at my patch so we can work out which one to use? > >>>> > >>>> my concern is that the "snps,reset-delays-us" property is optional, > >>>> the current dt-bindings documentation states that it's a required > >>>> property. in reality it isn't, there are boards (two examples are > >>>> mentioned in my patch: [0]) without it. > >>>> > >>>> so I believe that the resulting behavior has to be: > >>>> 1. don't delay if this property is missing (instead of delaying for > >>>> <garbage value> ms) > >>>> 2. don't error out if this property is missing > >>>> > >>>> your patch covers #1, can you please check whether #2 is also covered? > >>>> I tested case #2 when submitting my patch and it worked fine (even > >>>> though I could not reproduce the garbage values which are being read > >>>> on some boards) > > in the meantime I have tested your patch. > > when I don't set the "snps,reset-delays-us" property then I get the > > following error: > > invalid property snps,reset-delays-us > > > > my patch has landed in the meantime: [0] > > how should we proceed with your patch? > > I'm out of the office today. I'll get back to you on this tomorrow. gentle ping (I will be away for the weekend but I can reply on Monday)
On 28/06/2019 05:15, Martin Blumenstingl wrote: > On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote: >> >> On 25/06/2019 05:44, Martin Blumenstingl wrote: >>> Hi Colin, >>> >>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl >>> <martin.blumenstingl@googlemail.com> wrote: >>>> >>>> Hi Colin, >>>> >>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: >>>>> >>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote: >>>>>> Hi Colin, >>>>>> >>>>>>> Currently the call to device_property_read_u32_array is not error checked >>>>>>> leading to potential garbage values in the delays array that are then used >>>>>>> in msleep delays. Add a sanity check to the property fetching. >>>>>>> >>>>>>> Addresses-Coverity: ("Uninitialized scalar variable") >>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>>>> I have also sent a patch [0] to fix initialize the array. >>>>>> can you please look at my patch so we can work out which one to use? >>>>>> >>>>>> my concern is that the "snps,reset-delays-us" property is optional, >>>>>> the current dt-bindings documentation states that it's a required >>>>>> property. in reality it isn't, there are boards (two examples are >>>>>> mentioned in my patch: [0]) without it. >>>>>> >>>>>> so I believe that the resulting behavior has to be: >>>>>> 1. don't delay if this property is missing (instead of delaying for >>>>>> <garbage value> ms) >>>>>> 2. don't error out if this property is missing >>>>>> >>>>>> your patch covers #1, can you please check whether #2 is also covered? >>>>>> I tested case #2 when submitting my patch and it worked fine (even >>>>>> though I could not reproduce the garbage values which are being read >>>>>> on some boards) >>> in the meantime I have tested your patch. >>> when I don't set the "snps,reset-delays-us" property then I get the >>> following error: >>> invalid property snps,reset-delays-us >>> >>> my patch has landed in the meantime: [0] >>> how should we proceed with your patch? Your fix is good, so I think we should just drop/forget about my fix. Colin >> >> I'm out of the office today. I'll get back to you on this tomorrow. > gentle ping > (I will be away for the weekend but I can reply on Monday) >
Hi Colin, On Fri, Jun 28, 2019 at 10:32 AM Colin Ian King <colin.king@canonical.com> wrote: > > On 28/06/2019 05:15, Martin Blumenstingl wrote: > > On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote: > >> > >> On 25/06/2019 05:44, Martin Blumenstingl wrote: > >>> Hi Colin, > >>> > >>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl > >>> <martin.blumenstingl@googlemail.com> wrote: > >>>> > >>>> Hi Colin, > >>>> > >>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: > >>>>> > >>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote: > >>>>>> Hi Colin, > >>>>>> > >>>>>>> Currently the call to device_property_read_u32_array is not error checked > >>>>>>> leading to potential garbage values in the delays array that are then used > >>>>>>> in msleep delays. Add a sanity check to the property fetching. > >>>>>>> > >>>>>>> Addresses-Coverity: ("Uninitialized scalar variable") > >>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >>>>>> I have also sent a patch [0] to fix initialize the array. > >>>>>> can you please look at my patch so we can work out which one to use? > >>>>>> > >>>>>> my concern is that the "snps,reset-delays-us" property is optional, > >>>>>> the current dt-bindings documentation states that it's a required > >>>>>> property. in reality it isn't, there are boards (two examples are > >>>>>> mentioned in my patch: [0]) without it. > >>>>>> > >>>>>> so I believe that the resulting behavior has to be: > >>>>>> 1. don't delay if this property is missing (instead of delaying for > >>>>>> <garbage value> ms) > >>>>>> 2. don't error out if this property is missing > >>>>>> > >>>>>> your patch covers #1, can you please check whether #2 is also covered? > >>>>>> I tested case #2 when submitting my patch and it worked fine (even > >>>>>> though I could not reproduce the garbage values which are being read > >>>>>> on some boards) > >>> in the meantime I have tested your patch. > >>> when I don't set the "snps,reset-delays-us" property then I get the > >>> following error: > >>> invalid property snps,reset-delays-us > >>> > >>> my patch has landed in the meantime: [0] > >>> how should we proceed with your patch? > > Your fix is good, so I think we should just drop/forget about my fix. thank you for looking at the situation as far I understand the -net/-net-next tree all commits are immutable so if we want to remove your patch we need to send a revert do you want me to do that (I can do it on Monday) or will you take care of that? Martin
On Fri, Jun 28, 2019 at 6:05 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Colin, > > On Fri, Jun 28, 2019 at 10:32 AM Colin Ian King > <colin.king@canonical.com> wrote: > > > > On 28/06/2019 05:15, Martin Blumenstingl wrote: > > > On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote: > > >> > > >> On 25/06/2019 05:44, Martin Blumenstingl wrote: > > >>> Hi Colin, > > >>> > > >>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl > > >>> <martin.blumenstingl@googlemail.com> wrote: > > >>>> > > >>>> Hi Colin, > > >>>> > > >>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: > > >>>>> > > >>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote: > > >>>>>> Hi Colin, > > >>>>>> > > >>>>>>> Currently the call to device_property_read_u32_array is not error checked > > >>>>>>> leading to potential garbage values in the delays array that are then used > > >>>>>>> in msleep delays. Add a sanity check to the property fetching. > > >>>>>>> > > >>>>>>> Addresses-Coverity: ("Uninitialized scalar variable") > > >>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> > > >>>>>> I have also sent a patch [0] to fix initialize the array. > > >>>>>> can you please look at my patch so we can work out which one to use? > > >>>>>> > > >>>>>> my concern is that the "snps,reset-delays-us" property is optional, > > >>>>>> the current dt-bindings documentation states that it's a required > > >>>>>> property. in reality it isn't, there are boards (two examples are > > >>>>>> mentioned in my patch: [0]) without it. > > >>>>>> > > >>>>>> so I believe that the resulting behavior has to be: > > >>>>>> 1. don't delay if this property is missing (instead of delaying for > > >>>>>> <garbage value> ms) > > >>>>>> 2. don't error out if this property is missing > > >>>>>> > > >>>>>> your patch covers #1, can you please check whether #2 is also covered? > > >>>>>> I tested case #2 when submitting my patch and it worked fine (even > > >>>>>> though I could not reproduce the garbage values which are being read > > >>>>>> on some boards) > > >>> in the meantime I have tested your patch. > > >>> when I don't set the "snps,reset-delays-us" property then I get the > > >>> following error: > > >>> invalid property snps,reset-delays-us > > >>> > > >>> my patch has landed in the meantime: [0] > > >>> how should we proceed with your patch? > > > > Your fix is good, so I think we should just drop/forget about my fix. > thank you for looking at the situation > > as far I understand the -net/-net-next tree all commits are immutable > so if we want to remove your patch we need to send a revert > do you want me to do that (I can do it on Monday) or will you take care of that? I just sent the patch: [0] [0] https://patchwork.ozlabs.org/patch/1125686/
On 01/07/2019 23:43, Martin Blumenstingl wrote: > On Fri, Jun 28, 2019 at 6:05 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> >> Hi Colin, >> >> On Fri, Jun 28, 2019 at 10:32 AM Colin Ian King >> <colin.king@canonical.com> wrote: >>> >>> On 28/06/2019 05:15, Martin Blumenstingl wrote: >>>> On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote: >>>>> >>>>> On 25/06/2019 05:44, Martin Blumenstingl wrote: >>>>>> Hi Colin, >>>>>> >>>>>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl >>>>>> <martin.blumenstingl@googlemail.com> wrote: >>>>>>> >>>>>>> Hi Colin, >>>>>>> >>>>>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote: >>>>>>>> >>>>>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote: >>>>>>>>> Hi Colin, >>>>>>>>> >>>>>>>>>> Currently the call to device_property_read_u32_array is not error checked >>>>>>>>>> leading to potential garbage values in the delays array that are then used >>>>>>>>>> in msleep delays. Add a sanity check to the property fetching. >>>>>>>>>> >>>>>>>>>> Addresses-Coverity: ("Uninitialized scalar variable") >>>>>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>>>>>>> I have also sent a patch [0] to fix initialize the array. >>>>>>>>> can you please look at my patch so we can work out which one to use? >>>>>>>>> >>>>>>>>> my concern is that the "snps,reset-delays-us" property is optional, >>>>>>>>> the current dt-bindings documentation states that it's a required >>>>>>>>> property. in reality it isn't, there are boards (two examples are >>>>>>>>> mentioned in my patch: [0]) without it. >>>>>>>>> >>>>>>>>> so I believe that the resulting behavior has to be: >>>>>>>>> 1. don't delay if this property is missing (instead of delaying for >>>>>>>>> <garbage value> ms) >>>>>>>>> 2. don't error out if this property is missing >>>>>>>>> >>>>>>>>> your patch covers #1, can you please check whether #2 is also covered? >>>>>>>>> I tested case #2 when submitting my patch and it worked fine (even >>>>>>>>> though I could not reproduce the garbage values which are being read >>>>>>>>> on some boards) >>>>>> in the meantime I have tested your patch. >>>>>> when I don't set the "snps,reset-delays-us" property then I get the >>>>>> following error: >>>>>> invalid property snps,reset-delays-us >>>>>> >>>>>> my patch has landed in the meantime: [0] >>>>>> how should we proceed with your patch? >>> >>> Your fix is good, so I think we should just drop/forget about my fix. >> thank you for looking at the situation >> >> as far I understand the -net/-net-next tree all commits are immutable >> so if we want to remove your patch we need to send a revert >> do you want me to do that (I can do it on Monday) or will you take care of that? > I just sent the patch: [0] Thank you, much appreciated. > > > [0] https://patchwork.ozlabs.org/patch/1125686/ >
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index da310de06bf6..5b7923c0698c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -242,6 +242,7 @@ int stmmac_mdio_reset(struct mii_bus *bus) if (priv->device->of_node) { struct gpio_desc *reset_gpio; u32 delays[3]; + int ret; reset_gpio = devm_gpiod_get_optional(priv->device, "snps,reset", @@ -249,9 +250,15 @@ int stmmac_mdio_reset(struct mii_bus *bus) if (IS_ERR(reset_gpio)) return PTR_ERR(reset_gpio); - device_property_read_u32_array(priv->device, - "snps,reset-delays-us", - delays, ARRAY_SIZE(delays)); + ret = device_property_read_u32_array(priv->device, + "snps,reset-delays-us", + delays, + ARRAY_SIZE(delays)); + if (ret) { + dev_err(ndev->dev.parent, + "invalid property snps,reset-delays-us\n"); + return -EINVAL; + } if (delays[0]) msleep(DIV_ROUND_UP(delays[0], 1000));