Re: [PATCH RFC v2 0/6] staging: r8188eu: avoid uninit value bugs

[Pavel Skripkin]

On 8/23/21 3:12 AM, Phillip Potter wrote:
> On Sun, 22 Aug 2021 at 15:35, Pavel Skripkin <paskripkin@xxxxxxxxx> 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