Messages in this thread | | | Subject | Re: [PATCH RFC v2 0/6] staging: r8188eu: avoid uninit value bugs | From | Pavel Skripkin <> | Date | Mon, 23 Aug 2021 09:44:27 +0300 |
| |
On 8/23/21 3:12 AM, Phillip Potter wrote: > On Sun, 22 Aug 2021 at 15:35, Pavel Skripkin <paskripkin@gmail.com> wrote: >> >> Hi, Greg, Larry and Phillip! >> >> I noticed, that new staging driver was added like 3 weeks ago and I decided >> to look at the code, because drivers in staging directory are always buggy. >> >> The first thing I noticed is *no one* was checking read operations result, but >> it can fail and driver may start writing random stack values into registers. It >> can cause driver misbehavior or device misbehavior. >> >> To avoid this type of bugs, i've changed rtw_read* API. Now all rtw_read >> funtions return an error, when something went wrong with usb transfer. >> >> It helps callers to break/return earlier and don't write random values to >> registers or to rely on random values. >> >> Why is this pacth series RFC? >> 1. I don't have this device and I cannot test these changes. >> 2. I don't know how to handle errors in each particular case. For now, function >> just returns or returns an error. That's all. I hope, driver maintainers will >> help with these bits. >> 3. I guess, I handled not all uninit value bugs here. I hope, I fixed >> at least half of them >> >> >> v1 -> v2: >> 1. Make rtw_read*() return an error instead of initializing pointer to error >> 2. Split one huge patch to smaller ones for each rtw_read{8,16,32} function >> changes >> 3. Add new macro for printing register values (It helps to not copy-paste error >> handling) >> 4. Removed {read,write}_macreg (Suggested by Phillip) >> 5. Rebased on top of staging-next >> 6. Cleaned checkpatch errors and warnings >> >> Only build-tested, since I don't have device with r8118eu chip >> >> Pavel Skripkin (6): >> staging: r8188eu: remove {read,write}_macreg >> staging: r8188eu: add helper macro for printing registers >> staging: r8188eu: add error handling of rtw_read8 >> staging: r8188eu: add error handling of rtw_read16 >> staging: r8188eu: add error handling of rtw_read32 >> staging: r8188eu: make ReadEFuse return an int >> >> drivers/staging/r8188eu/core/rtw_debug.c | 79 +++- >> drivers/staging/r8188eu/core/rtw_efuse.c | 125 +++-- >> drivers/staging/r8188eu/core/rtw_io.c | 27 +- >> drivers/staging/r8188eu/core/rtw_mp.c | 70 ++- >> drivers/staging/r8188eu/core/rtw_mp_ioctl.c | 13 +- >> drivers/staging/r8188eu/core/rtw_pwrctrl.c | 5 +- >> drivers/staging/r8188eu/core/rtw_sreset.c | 9 +- >> .../r8188eu/hal/Hal8188ERateAdaptive.c | 8 +- >> drivers/staging/r8188eu/hal/HalPhyRf_8188e.c | 21 +- >> drivers/staging/r8188eu/hal/HalPwrSeqCmd.c | 9 +- >> drivers/staging/r8188eu/hal/hal_com.c | 23 +- >> drivers/staging/r8188eu/hal/hal_intf.c | 6 +- >> drivers/staging/r8188eu/hal/odm_interface.c | 12 +- >> drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 33 +- >> drivers/staging/r8188eu/hal/rtl8188e_dm.c | 6 +- >> .../staging/r8188eu/hal/rtl8188e_hal_init.c | 285 +++++++++--- >> drivers/staging/r8188eu/hal/rtl8188e_phycfg.c | 27 +- >> drivers/staging/r8188eu/hal/rtl8188e_sreset.c | 22 +- >> drivers/staging/r8188eu/hal/rtl8188eu_led.c | 18 +- >> drivers/staging/r8188eu/hal/usb_halinit.c | 439 +++++++++++++++--- >> drivers/staging/r8188eu/hal/usb_ops_linux.c | 57 ++- >> drivers/staging/r8188eu/include/hal_intf.h | 6 +- >> .../staging/r8188eu/include/odm_interface.h | 6 +- >> .../staging/r8188eu/include/rtl8188e_hal.h | 2 +- >> drivers/staging/r8188eu/include/rtw_debug.h | 13 + >> drivers/staging/r8188eu/include/rtw_efuse.h | 4 +- >> drivers/staging/r8188eu/include/rtw_io.h | 18 +- >> drivers/staging/r8188eu/include/rtw_mp.h | 2 - >> drivers/staging/r8188eu/os_dep/ioctl_linux.c | 179 +++++-- >> drivers/staging/r8188eu/os_dep/usb_intf.c | 3 +- >> 30 files changed, 1138 insertions(+), 389 deletions(-) >> >> -- >> 2.32.0 >> > > Dear Pavel, > > Thanks for this. I like the code a lot. One thing I am conflicted on > is the helper macro for the printing of register values though. Whilst > I'm not necessarily opposed to the concept of the macro itself, I > don't think it should rely on GlobalDebugLevel for one thing - if we > are going to control printing of messages at runtime then in my mind > this should be done via debugfs and pr_debug or similar - an in-kernel > mechanism rather than something driver-provided. Also, the example you > give of: > > u32 tmp; > if (!rtw_read(&tmp)) > DBG("reg = %d\n", tmp); > > Doesn't seem overly unclear to me if DBG was a pr_debug or similar, > but I get what you're saying about repetition. This is just a small > thing though, would be interested to see what others think. Many > thanks. >
Btw, did you have a chance to test these changes? My r8188eu device _should_ come in few days, so I am not able to test it right now :)
With regards, Pavel Skripkin
| |