Messages in this thread | | | Subject | Re: [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs | From | Pavel Skripkin <> | Date | Sat, 21 Aug 2021 13:42:30 +0300 |
| |
On 8/21/21 2:12 AM, Phillip Potter wrote: > On Fri, 2021-08-20 at 20:07 +0300, Pavel Skripkin 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 expanded read() API with error >> parametr, >> which will be initialized to error if read fails. 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 >> >> This series was build-tested and made on top of staging-testing >> branch >> >> >> With regards, >> Pavel Skripkin >> >> Pavel Skripkin (3): >> staging: r8188eu: add proper rtw_read* error handling >> staging: r8188eu: add error handling to ReadFuse >> staging: r8188eu: add error argument to read_macreg >> >> drivers/staging/r8188eu/core/rtw_debug.c | 79 +++- >> drivers/staging/r8188eu/core/rtw_efuse.c | 119 +++-- >> drivers/staging/r8188eu/core/rtw_io.c | 18 +- >> drivers/staging/r8188eu/core/rtw_mp.c | 38 +- >> drivers/staging/r8188eu/core/rtw_mp_ioctl.c | 20 +- >> drivers/staging/r8188eu/core/rtw_pwrctrl.c | 6 +- >> drivers/staging/r8188eu/core/rtw_sreset.c | 7 +- >> drivers/staging/r8188eu/hal/HalPwrSeqCmd.c | 9 +- >> drivers/staging/r8188eu/hal/hal_com.c | 22 +- >> drivers/staging/r8188eu/hal/hal_intf.c | 6 +- >> drivers/staging/r8188eu/hal/odm_interface.c | 12 +- >> drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 37 +- >> drivers/staging/r8188eu/hal/rtl8188e_dm.c | 6 +- >> .../staging/r8188eu/hal/rtl8188e_hal_init.c | 260 ++++++++--- >> drivers/staging/r8188eu/hal/rtl8188e_phycfg.c | 26 +- >> drivers/staging/r8188eu/hal/rtl8188e_sreset.c | 20 +- >> drivers/staging/r8188eu/hal/rtl8188eu_led.c | 17 +- >> drivers/staging/r8188eu/hal/usb_halinit.c | 412 ++++++++++++++-- >> -- >> drivers/staging/r8188eu/hal/usb_ops_linux.c | 55 ++- >> drivers/staging/r8188eu/include/hal_intf.h | 6 +- >> .../staging/r8188eu/include/rtl8188e_hal.h | 2 +- >> 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 | 168 +++++-- >> drivers/staging/r8188eu/os_dep/usb_intf.c | 4 +- >> 26 files changed, 1072 insertions(+), 301 deletions(-) >> > > Dear Pavel, > > Firstly, thank you for this contribution, it is much appreciated. > Whilst I'm still learning myself when it comes to this driver and to > kernel code in general, I can certainly say the code looks pretty good > in general so far. I will try and offer individual comments on each > patch. >
Thank you for your feedback. So, I will prepare a v2 version in few days and, I think, I will leave RFC prefix.
Also, I want to receive some feedback from Larry about error handling in each particular case, I guess, he can help us with it.
So, I will split each rtw_read* changes into separate patche and make them return an error instead of read value.
Again, big thanks to you and Fabio for feedback, I appreciate it :)
With regards, Pavel Skripkin
| |