* Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity [not found] ` <20210222132132.GU2222@kadam> @ 2021-02-22 13:42 ` karthek 2021-02-22 13:59 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: karthek @ 2021-02-22 13:42 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-kernel 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? > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity 2021-02-22 13:42 ` [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity karthek @ 2021-02-22 13:59 ` Dan Carpenter 2021-02-22 15:13 ` karthek 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2021-02-22 13:59 UTC (permalink / raw) To: karthek; +Cc: linux-kernel, devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity 2021-02-22 13:59 ` Dan Carpenter @ 2021-02-22 15:13 ` karthek 2021-02-22 16:46 ` karthek 0 siblings, 1 reply; 4+ messages in thread From: karthek @ 2021-02-22 15:13 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-kernel, devel 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity 2021-02-22 15:13 ` karthek @ 2021-02-22 16:46 ` karthek 0 siblings, 0 replies; 4+ messages in thread From: karthek @ 2021-02-22 16:46 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-kernel, devel 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-22 16:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <YDOnoLJzHYXMZBA/@karthik-strix-linux.karthek.com>
[not found] ` <20210222132132.GU2222@kadam>
2021-02-22 13:42 ` [PATCH] staging: wlan-ng/p80211 : check userspacebuf size for sanity karthek
2021-02-22 13:59 ` Dan Carpenter
2021-02-22 15:13 ` karthek
2021-02-22 16:46 ` karthek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox