| Message ID | 20200326060033.14550-1-sallenki@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: f_fs: Fix use after free issue as part of queue failure | expand |
On Thu, Mar 26, 2020 at 11:30:33AM +0530, Sriharsha Allenki wrote: > In AIO case, the request is freed up if ep_queue fails. > However, io_data->req still has the reference to this freed > request. In the case of this failure if there is aio_cancel > call on this io_data it will lead to an invalid dequeue > operation and a potential use after free issue. > Fix this by setting the io_data->req to NULL when the request > is freed as part of queue failure. > > Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> > --- > drivers/usb/gadget/function/f_fs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 571917677d35..767f30b86645 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > > ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); > if (unlikely(ret)) { > + io_data->req = NULL; > usb_ep_free_request(ep->ep, req); > goto error_lock; > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project What commit does this fix? Should it go to stable kernels, and if so, how far back? thanks, greg k-h
Hi Greg, On 3/26/2020 1:46 PM, Greg KH wrote: > On Thu, Mar 26, 2020 at 11:30:33AM +0530, Sriharsha Allenki wrote: >> In AIO case, the request is freed up if ep_queue fails. >> However, io_data->req still has the reference to this freed >> request. In the case of this failure if there is aio_cancel >> call on this io_data it will lead to an invalid dequeue >> operation and a potential use after free issue. >> Fix this by setting the io_data->req to NULL when the request >> is freed as part of queue failure. >> >> Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> >> --- >> drivers/usb/gadget/function/f_fs.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >> index 571917677d35..767f30b86645 100644 >> --- a/drivers/usb/gadget/function/f_fs.c >> +++ b/drivers/usb/gadget/function/f_fs.c >> @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) >> >> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); >> if (unlikely(ret)) { >> + io_data->req = NULL; >> usb_ep_free_request(ep->ep, req); >> goto error_lock; >> } >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project > What commit does this fix? Should it go to stable kernels, and if so, > how far back? The commit 2e4c7553cd6f ("usb: gadget: f_fs: add aio support") introduced the problem. It is good to have in stable kernels and it needs to go all theway back to linux-3.15.y. Hope this information helps. > > thanks, > > greg k-h Regards, Sriharsha
On Thu, Mar 26, 2020 at 02:14:23PM +0530, Sriharsha Allenki wrote: > Hi Greg, > > On 3/26/2020 1:46 PM, Greg KH wrote: > > On Thu, Mar 26, 2020 at 11:30:33AM +0530, Sriharsha Allenki wrote: > >> In AIO case, the request is freed up if ep_queue fails. > >> However, io_data->req still has the reference to this freed > >> request. In the case of this failure if there is aio_cancel > >> call on this io_data it will lead to an invalid dequeue > >> operation and a potential use after free issue. > >> Fix this by setting the io_data->req to NULL when the request > >> is freed as part of queue failure. > >> > >> Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> > >> --- > >> drivers/usb/gadget/function/f_fs.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > >> index 571917677d35..767f30b86645 100644 > >> --- a/drivers/usb/gadget/function/f_fs.c > >> +++ b/drivers/usb/gadget/function/f_fs.c > >> @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > >> > >> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); > >> if (unlikely(ret)) { > >> + io_data->req = NULL; > >> usb_ep_free_request(ep->ep, req); > >> goto error_lock; > >> } > >> -- > >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project > > What commit does this fix? Should it go to stable kernels, and if so, > > how far back? > The commit 2e4c7553cd6f ("usb: gadget: f_fs: add aio support") introduced > the problem. It is good to have in stable kernels and it needs to go all > theway back to linux-3.15.y. > > Hope this information helps. Great, please resend this with the proper Fixes: and cc: stable... tag on it so that it gets picked up properly. thanks, greg k-h
On 20-03-26 11:30:33, Sriharsha Allenki wrote: > In AIO case, the request is freed up if ep_queue fails. > However, io_data->req still has the reference to this freed > request. In the case of this failure if there is aio_cancel > call on this io_data it will lead to an invalid dequeue > operation and a potential use after free issue. > Fix this by setting the io_data->req to NULL when the request > is freed as part of queue failure. > > Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> Reviewed-by: Peter Chen <peter.chen@nxp.com> Peter > --- > drivers/usb/gadget/function/f_fs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 571917677d35..767f30b86645 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) > > ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); > if (unlikely(ret)) { > + io_data->req = NULL; > usb_ep_free_request(ep->ep, req); > goto error_lock; > } > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Greg, On 3/26/2020 3:00 PM, Greg KH wrote: > On Thu, Mar 26, 2020 at 02:14:23PM +0530, Sriharsha Allenki wrote: >> Hi Greg, >> >> On 3/26/2020 1:46 PM, Greg KH wrote: >>> On Thu, Mar 26, 2020 at 11:30:33AM +0530, Sriharsha Allenki wrote: >>>> In AIO case, the request is freed up if ep_queue fails. >>>> However, io_data->req still has the reference to this freed >>>> request. In the case of this failure if there is aio_cancel >>>> call on this io_data it will lead to an invalid dequeue >>>> operation and a potential use after free issue. >>>> Fix this by setting the io_data->req to NULL when the request >>>> is freed as part of queue failure. >>>> >>>> Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> >>>> --- >>>> drivers/usb/gadget/function/f_fs.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >>>> index 571917677d35..767f30b86645 100644 >>>> --- a/drivers/usb/gadget/function/f_fs.c >>>> +++ b/drivers/usb/gadget/function/f_fs.c >>>> @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) >>>> >>>> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); >>>> if (unlikely(ret)) { >>>> + io_data->req = NULL; >>>> usb_ep_free_request(ep->ep, req); >>>> goto error_lock; >>>> } >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project >>> What commit does this fix? Should it go to stable kernels, and if so, >>> how far back? >> The commit 2e4c7553cd6f ("usb: gadget: f_fs: add aio support") introduced >> the problem. It is good to have in stable kernels and it needs to go all >> theway back to linux-3.15.y. >> >> Hope this information helps. > Great, please resend this with the proper Fixes: and cc: stable... tag > on it so that it gets picked up properly. Thanks Greg for review. Sent out v2 patch with the updated tags. > thanks, > > greg k-h Regards, Sriharsha
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 571917677d35..767f30b86645 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -1120,6 +1120,7 @@ static ssize_t ffs_epfile_io(struct file *file, struct ffs_io_data *io_data) ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC); if (unlikely(ret)) { + io_data->req = NULL; usb_ep_free_request(ep->ep, req); goto error_lock; }
In AIO case, the request is freed up if ep_queue fails. However, io_data->req still has the reference to this freed request. In the case of this failure if there is aio_cancel call on this io_data it will lead to an invalid dequeue operation and a potential use after free issue. Fix this by setting the io_data->req to NULL when the request is freed as part of queue failure. Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> --- drivers/usb/gadget/function/f_fs.c | 1 + 1 file changed, 1 insertion(+)