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

[Pavel Skripkin]

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