linux-kernel - Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw

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]

Message-ID: <20210827092332.GI7722@kadam>
Date:   Fri, 27 Aug 2021 12:23:32 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     Larry.Finger@...inger.net, phil@...lpotter.co.uk,
        gregkh@...uxfoundation.org, straube.linux@...il.com,
        fmdefrancesco@...il.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8

On Fri, Aug 27, 2021 at 12:16:25PM +0300, Pavel Skripkin wrote:
> On 8/27/21 12:07 PM, Dan Carpenter wrote:
> > On Tue, Aug 24, 2021 at 10:27:27AM +0300, Pavel Skripkin wrote:
> > > @@ -83,7 +83,12 @@ int proc_get_read_reg(char *page, char **start,
> > >  	switch (proc_get_read_len) {
> > >  	case 1:
> > > -		len += snprintf(page + len, count - len, "rtw_read8(0x%x)=0x%x\n", proc_get_read_addr, rtw_read8(padapter, proc_get_read_addr));
> > > +		error = rtw_read8(padapter, proc_get_read_addr, (u8 *) &tmp);
> > > +		if (error)
> > > +			return len;
> > > +
> > > +		len += snprintf(page + len, count - len, "rtw_read8(0x%x)=0x%x\n",
> > > +				proc_get_read_addr, (u8) tmp);
> > >  		break;
> > 
> > Oh my goodness...  :P
> > 
> > If you look at what proc_get_read_addr is, it turns out it's a 32bit
> > address which is controlled by the user in proc_set_read_reg().  LOL!
> > Just a giant security hole.
> > 
> > My advise is just delete this dead code.  No one is using it so how
> > necessary can it be?
> 
> 
> Yep, it's dead code as was already mentioned. My plan is to convert all this
> code to sysfs.

I thought this was a good plan until a few minutes ago when I noticed it
was a ***GIANT SECURITY HOLE***!  :P

I mean presumably this is root controlled but these days we try to
restrict root as well in some ways.  Even confined to root only, this
code is incredibly risky and bad.  Don't convert it.  Just delete it.

regards,
dan carpenter