linux-kernel - Re: [PATCH 12/12] staging: r8188eu: hal_data

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 5 Dec 2021 20:23:14 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Michael Straube <straube.linux@...il.com>,
        gregkh@...uxfoundation.org
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/12] staging: r8188eu: hal_data_sz is set but never used

On 12/5/21 18:22, Michael Straube wrote:
>> Not related to your patch, but not returning an error from this function 
>>   looks very dangerous to me.
>> 
>> adapt->HalData is used in GET_HAL_DATA() macro all across the driver 
>> code and nobody checks if it valid or not. If allocation fails here, 
>> than we will likely hit GPF while accessing hal_data fields.
>> 
>> Maybe we can embed struct hal_data_8188e instead of storing a pointer to 
>> it?
> 
> Rethinking my prevoius answer, embedding struct hal_data_8188e is the
> better solution I think.
> 

I also think so. At least, it looks like this is very core structure and 
embedding it into struct adapter sounds more reasonable.

Also, it is one step further to make struct adapter more consistent. For 
now it looks confusing: some of privs are pointers and others are not:

struct adapter {
...
	struct dvobj_priv *dvobj;
	struct	mlme_priv mlmepriv;
	struct	mlme_ext_priv mlmeextpriv;
	struct	cmd_priv	cmdpriv;
	struct	evt_priv	evtpriv;
	struct	io_priv	iopriv;
	struct	xmit_priv	xmitpriv;
	struct	recv_priv	recvpriv;
	struct	sta_priv	stapriv;
	struct	security_priv	securitypriv;
	struct	registry_priv	registrypriv;
	struct	pwrctrl_priv	pwrctrlpriv;
	struct	eeprom_priv eeprompriv;
	struct	led_priv	ledpriv;
	struct	hostapd_priv	*phostapdpriv;
	struct wifidirect_info	wdinfo;

	void *HalData;

...

and even why Haldata is void *, but not struct hal_data_8188e * :)


so many questions and so few answers....



With regards,
Pavel Skripkin