Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity

Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity

[Dan Carpenter]

I have added the Driver Devel list to the CC list.  Adding linux-kernel
is sort of useless.  The correct people who are interested in this patch
are all on the Driver Devel list.

On Mon, Feb 22, 2021 at 07:12:22PM +0530, karthek wrote:
> On Mon, Feb 22, 2021 at 04:21:33PM +0300, Dan Carpenter wrote:
> > On Mon, Feb 22, 2021 at 06:16:24PM +0530, karthek wrote:
> > > currently p80211knetdev_do_ioctl() is testing user passed
> > > struct ifreq for sanity by checking for presence of a magic number,
> > > in addition to that also check size field, preventing buffer overflow
> > > before passing data to p80211req_dorequest() which casts it
> > > to *struct p80211msg
> > > 
> > > Signed-off-by: karthek <mail@karthek.com>
> > > ---
> > > is this correct?
> > > is it necessary to check for size in addition to magicnum?
> > > did i even understand the problem correctly?
> > > 
> > >  drivers/staging/wlan-ng/p80211netdev.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> > > index 70570e8a5..c7b78d870 100644
> > > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > > @@ -568,7 +568,10 @@ static int p80211knetdev_do_ioctl(struct net_device *dev,
> > >  		result = -EINVAL;
> > >  		goto bail;
> > >  	}
> > > -
> > > +	if (req->len < sizeof(struct p80211msg)) {
> > > +		result = -EINVAL;
> > > +		goto bail;
> > > +	}
> > 
> > Please don't send private emails.  Always CC the list.
> sorry
> > 
> > That's only a partial solution.  You need to check in p80211req_handlemsg()
> > as well and probably other places.
> currently p80211req_handlemsg() is only referenced in p80211req_dorequest()
> can we check that there instead?

If I have to do all the work in finding the buffer overflows, then I
should write my own patch.  :/

Anyway the p80211knetdev_do_ioctl() function calls p80211req_dorequest()
which calls p80211req_handlemsg(wlandev, msg); and
wlandev->mlmerequest(wlandev, msg);.

We have already discussed the p80211req_handlemsg() function.  The
wlandev->mlmerequest() function is always just prism2sta_mlmerequest().
The prism2sta_mlmerequest() calls a bunch of functions and each of those
functions need to have a different limit check added to prevent memory
corruption.

Homework #1: Should we get rid of the wlandev->mlmerequest() pointer
and just call prism2sta_mlmerequest() directly?

Homework #2: Another solution is to just delete all these custom IOCTLs.
I don't know what they do so I don't know if they are necessary or not.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity

[karthek]

On Mon, Feb 22, 2021 at 04:59:37PM +0300, Dan Carpenter wrote:
> I have added the Driver Devel list to the CC list.  Adding linux-kernel
> is sort of useless.  The correct people who are interested in this patch
> are all on the Driver Devel list.
> 
> On Mon, Feb 22, 2021 at 07:12:22PM +0530, karthek wrote:
> > On Mon, Feb 22, 2021 at 04:21:33PM +0300, Dan Carpenter wrote:
> > > On Mon, Feb 22, 2021 at 06:16:24PM +0530, karthek wrote:
> > > > currently p80211knetdev_do_ioctl() is testing user passed
> > > > struct ifreq for sanity by checking for presence of a magic number,
> > > > in addition to that also check size field, preventing buffer overflow
> > > > before passing data to p80211req_dorequest() which casts it
> > > > to *struct p80211msg
> > > > 
> > > > Signed-off-by: karthek <mail@karthek.com>
> > > > ---
> > > > is this correct?
> > > > is it necessary to check for size in addition to magicnum?
> > > > did i even understand the problem correctly?
> > > > 
> > > >  drivers/staging/wlan-ng/p80211netdev.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> > > > index 70570e8a5..c7b78d870 100644
> > > > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > > > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > > > @@ -568,7 +568,10 @@ static int p80211knetdev_do_ioctl(struct net_device *dev,
> > > >  		result = -EINVAL;
> > > >  		goto bail;
> > > >  	}
> > > > -
> > > > +	if (req->len < sizeof(struct p80211msg)) {
> > > > +		result = -EINVAL;
> > > > +		goto bail;
> > > > +	}
> > > 
> > > Please don't send private emails.  Always CC the list.
> > sorry
> > > 
> > > That's only a partial solution.  You need to check in p80211req_handlemsg()
> > > as well and probably other places.
> > currently p80211req_handlemsg() is only referenced in p80211req_dorequest()
> > can we check that there instead?
> 
> If I have to do all the work in finding the buffer overflows, then I
> should write my own patch.  :/
> 
> Anyway the p80211knetdev_do_ioctl() function calls p80211req_dorequest()
> which calls p80211req_handlemsg(wlandev, msg); and
> wlandev->mlmerequest(wlandev, msg);.
> 
> We have already discussed the p80211req_handlemsg() function.  The
> wlandev->mlmerequest() function is always just prism2sta_mlmerequest().
> The prism2sta_mlmerequest() calls a bunch of functions and each of those
> functions need to have a different limit check added to prevent memory
> corruption.
> 
> Homework #1: Should we get rid of the wlandev->mlmerequest() pointer
> and just call prism2sta_mlmerequest() directly?
yeah
> 
> Homework #2: Another solution is to just delete all these custom IOCTLs.
> I don't know what they do so I don't know if they are necessary or not.
i wish i could
> 
> regards,
> dan carpenter
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity

[karthek]

On Mon, Feb 22, 2021 at 08:43:51PM +0530, karthek wrote:
> On Mon, Feb 22, 2021 at 04:59:37PM +0300, Dan Carpenter wrote:
> > I have added the Driver Devel list to the CC list.  Adding linux-kernel
> > is sort of useless.  The correct people who are interested in this patch
> > are all on the Driver Devel list.
> > 
> > On Mon, Feb 22, 2021 at 07:12:22PM +0530, karthek wrote:
> > > On Mon, Feb 22, 2021 at 04:21:33PM +0300, Dan Carpenter wrote:
> > > > On Mon, Feb 22, 2021 at 06:16:24PM +0530, karthek wrote:
> > > > > currently p80211knetdev_do_ioctl() is testing user passed
> > > > > struct ifreq for sanity by checking for presence of a magic number,
> > > > > in addition to that also check size field, preventing buffer overflow
> > > > > before passing data to p80211req_dorequest() which casts it
> > > > > to *struct p80211msg
> > > > > 
> > > > > Signed-off-by: karthek <mail@karthek.com>
> > > > > ---
> > > > > is this correct?
> > > > > is it necessary to check for size in addition to magicnum?
> > > > > did i even understand the problem correctly?
> > > > > 
> > > > >  drivers/staging/wlan-ng/p80211netdev.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> > > > > index 70570e8a5..c7b78d870 100644
> > > > > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > > > > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > > > > @@ -568,7 +568,10 @@ static int p80211knetdev_do_ioctl(struct net_device *dev,
> > > > >  		result = -EINVAL;
> > > > >  		goto bail;
> > > > >  	}
> > > > > -
> > > > > +	if (req->len < sizeof(struct p80211msg)) {
> > > > > +		result = -EINVAL;
> > > > > +		goto bail;
> > > > > +	}
> > > > 
> > > > Please don't send private emails.  Always CC the list.
> > > sorry
> > > > 
> > > > That's only a partial solution.  You need to check in p80211req_handlemsg()
> > > > as well and probably other places.
> > > currently p80211req_handlemsg() is only referenced in p80211req_dorequest()
> > > can we check that there instead?
> > 
> > If I have to do all the work in finding the buffer overflows, then I
> > should write my own patch.  :/
> > 
> > Anyway the p80211knetdev_do_ioctl() function calls p80211req_dorequest()
> > which calls p80211req_handlemsg(wlandev, msg); and
> > wlandev->mlmerequest(wlandev, msg);.
> > 
> > We have already discussed the p80211req_handlemsg() function.  The
> > wlandev->mlmerequest() function is always just prism2sta_mlmerequest().
> > The prism2sta_mlmerequest() calls a bunch of functions and each of those
> > functions need to have a different limit check added to prevent memory
> > corruption.
other than this ioctl call codepath other codepaths leading to those
functions doesn't seem to get that msg(struct p80211req) from userspace
so may be they dont need that checking
anyway why do you think that is necessary?
> > 
> > Homework #1: Should we get rid of the wlandev->mlmerequest() pointer
> > and just call prism2sta_mlmerequest() directly?
> yeah
> > 
> > Homework #2: Another solution is to just delete all these custom IOCTLs.
> > I don't know what they do so I don't know if they are necessary or not.
> i wish i could
> > 
> > regards,
> > dan carpenter
> > 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel