Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8

[Dan Carpenter]

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