linux-kernel - Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 15 Jul 2022 22:11:50 +0200
From:   Martin Kaiser <martin@...ser.cx>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Michael Straube <straube.linux@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] staging: r888eu: use dynamic allocation for efuse buffer

Hi Pavel,

Thus wrote Pavel Skripkin (paskripkin@...il.com):

> Hi Martin,

> Martin Kaiser <martin@...ser.cx> says:
> > Use kmalloc to allocate the efuse buffer in ReadAdapterInfo8188EU and
> > free it on exit. This is better than using a 512 byte array on the stack.

> > It's ok to drop the __aligned(4) qualifier. kmalloc aligns to
> > ARCH_KMALLOC_MINALIGN, this is at least 8 bytes.

> > Suggested-by: Dan Carpenter <dan.carpenter@...cle.com>
> > Suggested-by: Larry Finger <Larry.Finger@...inger.net>
> > Signed-off-by: Martin Kaiser <martin@...ser.cx>
> > ---
> >   drivers/staging/r8188eu/hal/usb_halinit.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)

> > diff --git a/drivers/staging/r8188eu/hal/usb_halinit.c b/drivers/staging/r8188eu/hal/usb_halinit.c
> > index 8902dda7b8d8..421fe7c40390 100644
> > --- a/drivers/staging/r8188eu/hal/usb_halinit.c
> > +++ b/drivers/staging/r8188eu/hal/usb_halinit.c
> > @@ -926,7 +926,7 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> >   {
> >   	struct eeprom_priv *eeprom = &Adapter->eeprompriv;
> >   	struct led_priv *ledpriv = &Adapter->ledpriv;
> > -	u8 efuse_buf[EFUSE_MAP_LEN_88E] __aligned(4);
> > +	u8 *efuse_buf;
> >   	u8 eeValue;
> >   	int res;
> > @@ -937,7 +937,10 @@ void ReadAdapterInfo8188EU(struct adapter *Adapter)
> >   	eeprom->bautoload_fail_flag	= !(eeValue & EEPROM_EN);
> > -	memset(efuse_buf, 0xFF, sizeof(efuse_buf));
> > +	efuse_buf = kmalloc(EFUSE_MAP_LEN_88E, GFP_KERNEL);
> > +	if (!efuse_buf)
> > +		return;

> I think, it worth returning an error to caller. Functions right after the
> allocation do initialization, so leaving fields as-is seems to be dangerous

yes, that makes sense. We could refuse to load the driver in this case.

Larry and Greg already accepted the patch as is, I'll add error handling
in a separate patch.

Thanks,
   Martin