| Message ID | 20220330164902.431732-1-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | IOMUX: Fix access past end of console_devices | expand |
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: > > We should only access console_devices[file][i] once we have checked that > i < cd_count[file]. Otherwise, we will access uninitialized memory at the end of > the loop. console_devices[file][i] should not be NULL, but putting the > assignment in the loop condition allows us to ensure that i is checked > beforehand. This isn't a bug, but it does make valgrind stop complaining. > Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro") Has this been tested? See below. ... > #define for_each_console_dev(i, file, dev) \ > - for (i = 0, dev = console_devices[file][i]; \ When we enter the loop, the dev is assigned and perhaps valid > - i < cd_count[file]; \ > - i++, dev = console_devices[file][i]) > + for (i = 0; i < cd_count[file] && \ Not the case anymore. > + (dev = console_devices[file][i]); i++)
On 3/30/22 1:01 PM, Andy Shevchenko wrote: > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: >> >> We should only access console_devices[file][i] once we have checked that >> i < cd_count[file]. Otherwise, we will access uninitialized memory at the end of >> the loop. console_devices[file][i] should not be NULL, but putting the >> assignment in the loop condition allows us to ensure that i is checked >> beforehand. This isn't a bug, but it does make valgrind stop complaining. > >> Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro") > > Has this been tested? See below. Yes. > ... > >> #define for_each_console_dev(i, file, dev) \ >> - for (i = 0, dev = console_devices[file][i]; \ > > When we enter the loop, the dev is assigned and perhaps valid > >> - i < cd_count[file]; \ >> - i++, dev = console_devices[file][i]) >> + for (i = 0; i < cd_count[file] && \ > > Not the case anymore. The loop condition is evaluated before we enter the loop, which includes the first assignment to dev. --Sean >> + (dev = console_devices[file][i]); i++) > > >
On Wed, Mar 30, 2022 at 8:01 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: ... > > #define for_each_console_dev(i, file, dev) \ > > - for (i = 0, dev = console_devices[file][i]; \ > > When we enter the loop, the dev is assigned and perhaps valid > > > - i < cd_count[file]; \ > > - i++, dev = console_devices[file][i]) > > + for (i = 0; i < cd_count[file] && \ > > Not the case anymore. > > > + (dev = console_devices[file][i]); i++) On the second look, it seems a bit unusual, but for loop checks the condition before entering and in such case the dev will be assigned if the count is greater than 0. So, basically the difference is that dev is left completely uninitialized in case of count==0. However, it may not be a problem. Anyways, I would rather see better written for-loop that we see the iterations
On Wed, Mar 30, 2022 at 8:05 PM Sean Anderson <seanga2@gmail.com> wrote: > On 3/30/22 1:01 PM, Andy Shevchenko wrote: > > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: ... > >> #define for_each_console_dev(i, file, dev) \ > >> - for (i = 0, dev = console_devices[file][i]; \ > > > > When we enter the loop, the dev is assigned and perhaps valid > > > >> - i < cd_count[file]; \ > >> - i++, dev = console_devices[file][i]) > >> + for (i = 0; i < cd_count[file] && \ > > > > Not the case anymore. > > The loop condition is evaluated before we enter the loop, > which includes the first assignment to dev. Yeah, I just sent a reply to my reply :-) > >> + (dev = console_devices[file][i]); i++) So, what I don't like is exactly this hidenness, which I have stumbled upon.
On 3/30/22 1:07 PM, Andy Shevchenko wrote: > On Wed, Mar 30, 2022 at 8:01 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: > > ... > >>> #define for_each_console_dev(i, file, dev) \ >>> - for (i = 0, dev = console_devices[file][i]; \ >> >> When we enter the loop, the dev is assigned and perhaps valid >> >>> - i < cd_count[file]; \ >>> - i++, dev = console_devices[file][i]) >>> + for (i = 0; i < cd_count[file] && \ >> >> Not the case anymore. >> >>> + (dev = console_devices[file][i]); i++) > > On the second look, it seems a bit unusual, but for loop checks the > condition before entering and in such case the dev will be assigned if > the count is greater than 0. > So, basically the difference is that dev is left completely > uninitialized in case of count==0. However, it may not be a problem. Well, in such a case, the value of console_devices[file][i] is bogus anyway, so stack garbage is just as good. If it turns out to be a problem, this can always be rewritten to for (i = 0 dev = NULL; i < cd_count[file] && \ (dev = console_devices[file][i]); i++) > Anyways, I would rather see better written for-loop that we see the iterations Sorry, I'm not sure what you mean by this... Ideally this loop would be written like for (i = 0 dev = NULL; i < cd_count[file]; i++) { dev = console_devices[file][i]); /* loop body */ } which is much more obviously correct. But since this macro must use the following block as the loop body, it's trickier to do. --Sean
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: Also I don't like to have workarounds for the broken tools. But if you still want to have something, what about rather this > #define for_each_console_dev(i, file, dev) \ > - for (i = 0, dev = console_devices[file][i]; \ > - i < cd_count[file]; \ > - i++, dev = console_devices[file][i]) > + for (i = 0; i < cd_count[file] && \ > + (dev = console_devices[file][i]); i++) for (i = 0, dev = console_devices[file][0]; \ i < cd_count[file]; \ i++, dev = console_devices[file][i]) ? Or if it's still complains for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; \ i < cd_count[file]; \ i++, dev = console_devices[file][i]) ?
On 3/30/22 1:13 PM, Andy Shevchenko wrote: > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: > > Also I don't like to have workarounds for the broken tools. > But if you still want to have something, what about rather this > >> #define for_each_console_dev(i, file, dev) \ >> - for (i = 0, dev = console_devices[file][i]; \ >> - i < cd_count[file]; \ >> - i++, dev = console_devices[file][i]) >> + for (i = 0; i < cd_count[file] && \ >> + (dev = console_devices[file][i]); i++) > > for (i = 0, dev = console_devices[file][0]; \ > i < cd_count[file]; \ > i++, dev = console_devices[file][i]) > > ? > > Or if it's still complains > > for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; \ > i < cd_count[file]; \ > i++, dev = console_devices[file][i]) > > ? > The problem is not the first assignment but the last. Consider the case when cd_count[file] = 1 i = 0, dev = console_devices[file][0]; // OK i < cd_count[file] // 0 < 1 // loop body i++, dev = console_devices[file][1] // Oops, past the end i < cd_count[file] // 1 < 1, loop exit --Sean
On Wed, Mar 30, 2022 at 01:25:59PM -0400, Sean Anderson wrote: > On 3/30/22 1:13 PM, Andy Shevchenko wrote: > > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson <seanga2@gmail.com> wrote: > > > > Also I don't like to have workarounds for the broken tools. > > But if you still want to have something, what about rather this > > > > > #define for_each_console_dev(i, file, dev) \ > > > - for (i = 0, dev = console_devices[file][i]; \ > > > - i < cd_count[file]; \ > > > - i++, dev = console_devices[file][i]) > > > + for (i = 0; i < cd_count[file] && \ > > > + (dev = console_devices[file][i]); i++) > > > > for (i = 0, dev = console_devices[file][0]; \ > > i < cd_count[file]; \ > > i++, dev = console_devices[file][i]) > > > > ? > > > > Or if it's still complains > > > > for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; \ > > i < cd_count[file]; \ > > i++, dev = console_devices[file][i]) > > > > ? > > The problem is not the first assignment but the last. Consider the case when cd_count[file] = 1 Following that logic the first one is also problematic when cd_count[file] == 0. > i = 0, dev = console_devices[file][0]; // OK > i < cd_count[file] // 0 < 1 > // loop body > i++, dev = console_devices[file][1] // Oops, past the end > i < cd_count[file] // 1 < 1, loop exit I don't see good solution. :-( Maybe moving to dev as a loop variable and having NULL terminated arrays should fix that. For now probably your solution is a good compromise. But, please rewrite it to have all three more visible in a for-loop: for (i = 0; \ i < cd_count[file] && (dev = console_devices[file][i]); \ i++)
I found this same problem with some ASAN experiments I've been playing with, and addressed it in the same way. > I don't see good solution. :-( > There's always the option of reverting the refactor as it applies to just a small number of cases and is making it harder to write the correct logic. > Maybe moving to dev as a loop variable and having NULL terminated arrays > should fix that. > Doing this just to fix the syntactic sugar feels like addressing the wrong problem. > For now probably your solution is a good compromise. > But, please rewrite it to have all three more visible in a for-loop: > > for (i = 0; \ > i < cd_count[file] && (dev = console_devices[file][i]); \ > i++) > With reformatting Reviewed-by: Andrew Scull <ascull@google.com>
diff --git a/include/iomux.h b/include/iomux.h index 37f5f6dee6..1046df133b 100644 --- a/include/iomux.h +++ b/include/iomux.h @@ -25,9 +25,8 @@ extern struct stdio_dev **console_devices[MAX_FILES]; extern int cd_count[MAX_FILES]; #define for_each_console_dev(i, file, dev) \ - for (i = 0, dev = console_devices[file][i]; \ - i < cd_count[file]; \ - i++, dev = console_devices[file][i]) + for (i = 0; i < cd_count[file] && \ + (dev = console_devices[file][i]); i++) int iomux_match_device(struct stdio_dev **, const int, struct stdio_dev *); int iomux_doenv(const int, const char *);
We should only access console_devices[file][i] once we have checked that i < cd_count[file]. Otherwise, we will access uninitialized memory at the end of the loop. console_devices[file][i] should not be NULL, but putting the assignment in the loop condition allows us to ensure that i is checked beforehand. This isn't a bug, but it does make valgrind stop complaining. Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro") Signed-off-by: Sean Anderson <seanga2@gmail.com> --- include/iomux.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)