When starting up FxOS on Nexus5, Valgrind reports uses of strstr on undefined values in update_ctrl_interface. On looking at how |pbuf| is used, I don't see any place where the string in |pbuf| is zero terminated before it is given to strstr. So I guess that Valgrind is complaining about the second call to strstr falling off the end of the string.
This is gecko in the b2g world. I don't think I have the right component for this, but not sure what I should set it to.
(In reply to Julian Seward [:jseward] from comment #1) > This is gecko in the b2g world. Duh, sorry. Obviously not gecko. I just meant, this is in the b2g tree.
Possibly related to bug 1000982 - crash in strstr | libhardware_legacy.so@0x2481
Created attachment 8551773 [details] [diff] [review] a possible fix This patch ensures that |pbuf| contains no undefined bytes and is always zero terminated. Stops Valgrind complaining. No idea if it is really correct -- I don't know what steps I should take to test it. To whom should I offer it up for review?
Fabrice do you know who knows stuff about this code and could look at the patch? It would also be good to know how much of a security problem this could be in practice.
I think Michael has some experience with upstream bugs.
Buffer overflow sounds bad, so I'm going to mark this sec-high.
This happens also on Flame (flame-kk).
Comment on attachment 8551773 [details] [diff] [review] a possible fix Review of attachment 8551773 [details] [diff] [review]: ----------------------------------------------------------------- ::: wifi/wifi.c @@ +350,5 @@ > } > > + /* Make sure pbuf is zero terminated. Since pbuf_size > sb.st_size, > + this shouldn't overwrite any data read from srcfd. */ > + pbuf[pbuf_size-1] = '\0'; I think that you should still use malloc instead of calloc, and put the null at pbuf[nread]. Then you don't need the +1 either. No strstr calls are made after inserting the new value, and the null is only needed for the strstr call.
I don't think this is a particularly bad bug - this would happen if we read a wifi configuration file that was tampered with, but neither apps nor the user are normally allowed to directly modify the wifi config files. A user would need root to do so. Worth fixing though.
Comment on attachment 8551773 [details] [diff] [review] a possible fix Review of attachment 8551773 [details] [diff] [review]: ----------------------------------------------------------------- I agree with dhylands - Setting pbuf[nread] to null should be enough. BTW, unrelated to your patch, the read logic in this function looks wrong to me - read might only read part of the file, and we need to loop it to ensure that the entire file is read. TEMP_FAILURE_RETRY isn't enough. ::: wifi/wifi.c @@ +331,4 @@ > if (stat(config_file, &sb) != 0) > return -1; > > + size_t pbuf_size = sb.st_size + PROPERTY_VALUE_MAX + 1; PROPERTY_VALUE_MAX actually accounts for the null string terminator, so real property values can only be up to PROPERTY_VALUE_MAX - 1. So the + 1 shouldn't be necessary.
Julian, is there progress on the patch?
Will try to get this wrapped up early next week.
Julian, any progress here? Should we look for somebody else to work on this? It has been sitting around for a few months. Thanks.
I am not seeing these errors in recent b2g runs any more. And it looks as if the bug got fixed. Somewhere. Upstream? It would be nice to be able to track down the original changeset and/or bug report, as a cross-check, but I can't figure out how. Do you know? Anyway, what I see from git blame is: 363 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 363) if(nread < sb.st_size) 364 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 364) pbuf[nread]='\0'; 365 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 365) else 366 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 366) pbuf[sb.st_size]='\0'; 367 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 367) ALOGD("value read from %s file is %s", config_file, pbuf); 368 cac4f50c (Tapas Kumar Kundu 2014-07-02 06:43:30 -0700 368) which afaics does provide the required zero termination.
Ah, this is more complex than it seems. A tree configured for flame-kk has a wifi.c that contains the fix shown in comment 16. But a tree configured for nexus-5 doesn't, and I still see the Valgrind complaints. What is the right thing to do? Should we put the comment 16 fix into the nexus-5 version of wifi.c? Looking at the link in comment 17, it's pretty clear that the lack of buffer termination caused a segfault at some point.
Created attachment 8610538 [details] [diff] [review] bug1123636-2.diff The comment 17 patch, as it applies to our tree.
Per some digging by MWu, it appears that the entire function update_ctrl_interface has been removed in Android 5.1 upstream. Closing, therefore.