* [PATCH 0/3] usb: gadget: dfu: Fix timeout on erase when using big partition @ 2020-03-24 15:46 Jules Maselbas 2020-03-24 15:46 ` [PATCH 1/3] usb: gadget: dfu: Add manifestation phase Jules Maselbas ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Jules Maselbas @ 2020-03-24 15:46 UTC (permalink / raw) To: barebox; +Cc: Jules Maselbas Hi, The two first patch are focused on two fixes for an issue I encountered when trying to use the DFU gadget on big flash partition (2MB). You can find more information in each patch, but here is a short summary of the issue: When downloading a firmware into a big flash partition the erase operation can take a long time to be complete from few seconds to minutes in extreme cases. During the erase the DFU gadget does not respond to any USB setup request, the host only see a stalled USB endpoint and cannot get responses from DFU_GETSTATE nor DFU_GETSTATUS. The last patch is a small fix that allow to use the dfu gadget after being unbind, such as: barebox:/ usbgadget -D /foo(foo) barebox:/ usbgadget -d barebox:/ usbgadget -D /bar(bar) Best regards, Jules --- Jules Maselbas (3): usb: gadget: dfu: Add manifestation phase usb: gadget: dfu: Progressive erase if file is a mtd usb: gadget: dfu: Reset global variables on unbind drivers/usb/gadget/dfu.c | 207 +++++++++++++++++++++++++++++---------- 1 file changed, 156 insertions(+), 51 deletions(-) -- 2.21.0.196.g041f5ea _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] usb: gadget: dfu: Add manifestation phase 2020-03-24 15:46 [PATCH 0/3] usb: gadget: dfu: Fix timeout on erase when using big partition Jules Maselbas @ 2020-03-24 15:46 ` Jules Maselbas 2020-03-24 15:46 ` [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd Jules Maselbas 2020-03-24 15:46 ` [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind Jules Maselbas 2 siblings, 0 replies; 7+ messages in thread From: Jules Maselbas @ 2020-03-24 15:46 UTC (permalink / raw) To: barebox; +Cc: Jules Maselbas When downloading a firmware into a big flash partition the erase operation can take a long time to be complete from few seconds to minutes in extreme cases. During the erase the DFU gadget does not respond to any USB setup request, the host only see a stalled USB endpoint and cannot get responses from DFU_GETSTATE nor DFU_GETSTATUS. After 5 seconds without any updates the host will abort the DFU transfer and return an error (when using dfu-util). When using the safe flag on the partition the file is first downloaded in memory. The partition is only erased when the last chunk is received if the partition takes more than 5 seconds to be erased then the transfer fails. This patch fix this issue by using the manifestation phase for safe partitions. It's used to inform the host that the transfer is complete and that the device will now be doing the actual erasing and writing. Not all devices will be able to respond to DFU_GETSTATUS when in the dfuMANIFEST state (from DFU-1.1 specification page 22). This is far from perfect as the utility dfu-util will exit before the erase/writing is done, but it will not report an error status if a timeout happen during this phase. Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> --- drivers/usb/gadget/dfu.c | 179 ++++++++++++++++++++++++++++----------- 1 file changed, 128 insertions(+), 51 deletions(-) diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c index c2b3d481a..fef6f3e3a 100644 --- a/drivers/usb/gadget/dfu.c +++ b/drivers/usb/gadget/dfu.c @@ -342,38 +342,14 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c struct f_dfu *dfu = func_to_dfu(f); struct usb_composite_dev *cdev = f->config->cdev; u16 w_length = le16_to_cpu(ctrl->wLength); - int ret; if (w_length == 0) { - dfu->dfu_state = DFU_STATE_dfuIDLE; if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) { - int fd; - unsigned flags = O_WRONLY; - - if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE) - flags |= O_CREAT | O_TRUNC; - - fd = open(dfu_file_entry->filename, flags); - if (fd < 0) { - perror("open"); - ret = -EINVAL; - goto err_out; - } - ret = erase(fd, ERASE_SIZE_ALL, 0); - close(fd); - if (ret && ret != -ENOSYS) { - perror("erase"); - ret = -EINVAL; - goto err_out; - } - ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0); - if (ret) { - printf("copy file failed\n"); - ret = -EINVAL; - goto err_out; - } + dfu->dfu_state = DFU_STATE_dfuMANIFEST; + } else { + dfu->dfu_state = DFU_STATE_dfuIDLE; + dfu_cleanup(dfu); } - dfu_cleanup(dfu); return 0; } @@ -381,9 +357,52 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c dfu->dnreq->context = dfu; usb_ep_queue(cdev->gadget->ep0, dfu->dnreq); + return 0; +} + +static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl) +{ + struct f_dfu *dfu = func_to_dfu(f); + int ret; + + dfu->dfu_state = DFU_STATE_dfuIDLE; + + if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) { + int fd; + unsigned flags = O_WRONLY; + + if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE) + flags |= O_CREAT | O_TRUNC; + + fd = open(dfu_file_entry->filename, flags); + if (fd < 0) { + perror("open"); + dfu->dfu_status = DFU_STATUS_errERASE; + ret = -EINVAL; + goto out; + } + + ret = erase(fd, ERASE_SIZE_ALL, 0); + close(fd); + if (ret && ret != -ENOSYS) { + dfu->dfu_status = DFU_STATUS_errERASE; + perror("erase"); + goto out; + } + + ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0); + if (ret) { + printf("copy file failed\n"); + ret = -EINVAL; + goto out; + } + } + return 0; -err_out: +out: + dfu->dfu_status = DFU_STATUS_errWRITE; + dfu->dfu_state = DFU_STATE_dfuERROR; dfu_cleanup(dfu); return ret; } @@ -438,23 +457,17 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) goto out; } - /* Allow GETSTATUS in every state */ - if (ctrl->bRequest == USB_REQ_DFU_GETSTATUS) { - value = dfu_status(f, ctrl); - value = min(value, w_length); - goto out; - } - - /* Allow GETSTATE in every state */ - if (ctrl->bRequest == USB_REQ_DFU_GETSTATE) { - *(u8 *)req->buf = dfu->dfu_state; - value = sizeof(u8); - goto out; - } - switch (dfu->dfu_state) { case DFU_STATE_dfuIDLE: switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; case USB_REQ_DFU_DNLOAD: if (w_length == 0) { dfu->dfu_state = DFU_STATE_dfuERROR; @@ -479,11 +492,13 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) goto out; } - ret = erase(dfufd, ERASE_SIZE_ALL, 0); - if (ret && ret != -ENOSYS) { - dfu->dfu_status = DFU_STATUS_errERASE; - perror("erase"); - goto out; + if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) { + ret = erase(dfufd, ERASE_SIZE_ALL, 0); + if (ret && ret != -ENOSYS) { + dfu->dfu_status = DFU_STATUS_errERASE; + perror("erase"); + goto out; + } } value = handle_dnload(f, ctrl); @@ -520,9 +535,17 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) break; case DFU_STATE_dfuDNLOAD_IDLE: switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; case USB_REQ_DFU_DNLOAD: value = handle_dnload(f, ctrl); - if (dfu->dfu_state != DFU_STATE_dfuIDLE) { + if (dfu->dfu_state == DFU_STATE_dfuDNLOAD_IDLE) { return 0; } break; @@ -538,6 +561,14 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) break; case DFU_STATE_dfuUPLOAD_IDLE: switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; case USB_REQ_DFU_UPLOAD: handle_upload(f, ctrl); return 0; @@ -553,6 +584,14 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) break; case DFU_STATE_dfuERROR: switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; case USB_REQ_DFU_CLRSTATUS: dfu_abort(dfu); /* no zlp? */ @@ -564,10 +603,48 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) break; } break; - case DFU_STATE_dfuDNLOAD_SYNC: - case DFU_STATE_dfuDNBUSY: case DFU_STATE_dfuMANIFEST_SYNC: + switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) + dfu->dfu_state = DFU_STATE_dfuMANIFEST; + else + dfu->dfu_state = DFU_STATE_dfuIDLE; + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; + default: + dfu->dfu_state = DFU_STATE_dfuERROR; + value = -EINVAL; + break; + } + break; case DFU_STATE_dfuMANIFEST: + value = handle_manifest(f, ctrl); + if (dfu->dfu_state != DFU_STATE_dfuIDLE) { + return 0; + } + switch (ctrl->bRequest) { + case USB_REQ_DFU_GETSTATUS: + value = dfu_status(f, ctrl); + value = min(value, w_length); + break; + case USB_REQ_DFU_GETSTATE: + *(u8 *)req->buf = dfu->dfu_state; + value = sizeof(u8); + break; + default: + dfu->dfu_state = DFU_STATE_dfuERROR; + value = -EINVAL; + break; + } + break; + case DFU_STATE_dfuDNLOAD_SYNC: + case DFU_STATE_dfuDNBUSY: dfu->dfu_state = DFU_STATE_dfuERROR; value = -EINVAL; break; -- 2.21.0.196.g041f5ea _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd 2020-03-24 15:46 [PATCH 0/3] usb: gadget: dfu: Fix timeout on erase when using big partition Jules Maselbas 2020-03-24 15:46 ` [PATCH 1/3] usb: gadget: dfu: Add manifestation phase Jules Maselbas @ 2020-03-24 15:46 ` Jules Maselbas 2020-03-26 6:03 ` Sascha Hauer 2020-03-24 15:46 ` [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind Jules Maselbas 2 siblings, 1 reply; 7+ messages in thread From: Jules Maselbas @ 2020-03-24 15:46 UTC (permalink / raw) To: barebox; +Cc: Jules Maselbas When downloading a firmware into a big flash partition the erase operation can take a long time to be complete from few seconds to minutes in extreme cases. During the erase the DFU gadget does not respond to any USB setup request, the host only see a stalled USB endpoint and cannot get responses from DFU_GETSTATE nor DFU_GETSTATUS. After 5 seconds without any updates the host will abort the DFU transfer and return an error (when using dfu-util). For instance I have a 2MB partition that takes 25 seconds to erase, this erase cannot be done in one step as it takes too much time or it should be done in the manifestation phase, see previous patch. This patch modify the erase behavior when downloading a new firmware. If the updated file is a mtd partition then the DFU gadget will do a progressive erase by erasing the least amount of required blocks before writing a new chunk into the mtd device. Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> --- drivers/usb/gadget/dfu.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c index fef6f3e3a..592586db1 100644 --- a/drivers/usb/gadget/dfu.c +++ b/drivers/usb/gadget/dfu.c @@ -55,6 +55,8 @@ #include <libbb.h> #include <init.h> #include <fs.h> +#include <ioctl.h> +#include <linux/mtd/mtd-abi.h> #define USB_DT_DFU 0x21 @@ -132,6 +134,10 @@ struct file_list_entry *dfu_file_entry; static int dfufd = -EINVAL; static struct file_list *dfu_files; static int dfudetach; +static struct mtd_info_user dfu_mtdinfo; +static loff_t dfu_written; +static loff_t dfu_erased; +static int prog_erase; /* USB DFU functional descriptor */ static struct usb_dfu_func_descriptor usb_dfu_func = { @@ -327,8 +333,16 @@ static void dfu_cleanup(struct f_dfu *dfu) static void dn_complete(struct usb_ep *ep, struct usb_request *req) { struct f_dfu *dfu = req->context; + loff_t size; int ret; + if (prog_erase && (dfu_written + req->length) > dfu_erased) { + size = roundup(req->length, dfu_mtdinfo.erasesize); + erase(dfufd, size, dfu_erased); + dfu_erased += size; + } + + dfu_written += req->length; ret = write(dfufd, req->buf, req->length); if (ret < (int)req->length) { perror("write"); @@ -493,7 +507,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) } if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) { - ret = erase(dfufd, ERASE_SIZE_ALL, 0); + ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo); + if (ret) /* not a mtd */ + ret = erase(dfufd, ERASE_SIZE_ALL, 0); + else + prog_erase = 1; + if (ret && ret != -ENOSYS) { dfu->dfu_status = DFU_STATUS_errERASE; perror("erase"); -- 2.21.0.196.g041f5ea _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd 2020-03-24 15:46 ` [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd Jules Maselbas @ 2020-03-26 6:03 ` Sascha Hauer 2020-03-26 9:08 ` Jules Maselbas 0 siblings, 1 reply; 7+ messages in thread From: Sascha Hauer @ 2020-03-26 6:03 UTC (permalink / raw) To: Jules Maselbas; +Cc: barebox Hi Jules, On Tue, Mar 24, 2020 at 04:46:46PM +0100, Jules Maselbas wrote: > When downloading a firmware into a big flash partition the erase > operation can take a long time to be complete from few seconds to > minutes in extreme cases. During the erase the DFU gadget does not > respond to any USB setup request, the host only see a stalled USB > endpoint and cannot get responses from DFU_GETSTATE nor DFU_GETSTATUS. > > After 5 seconds without any updates the host will abort the DFU > transfer and return an error (when using dfu-util). > > For instance I have a 2MB partition that takes 25 seconds to erase, > this erase cannot be done in one step as it takes too much time or it > should be done in the manifestation phase, see previous patch. > > This patch modify the erase behavior when downloading a new firmware. > If the updated file is a mtd partition then the DFU gadget will do > a progressive erase by erasing the least amount of required blocks > before writing a new chunk into the mtd device. > > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> > --- > drivers/usb/gadget/dfu.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c > index fef6f3e3a..592586db1 100644 > --- a/drivers/usb/gadget/dfu.c > +++ b/drivers/usb/gadget/dfu.c > @@ -55,6 +55,8 @@ > #include <libbb.h> > #include <init.h> > #include <fs.h> > +#include <ioctl.h> > +#include <linux/mtd/mtd-abi.h> > > #define USB_DT_DFU 0x21 > > @@ -132,6 +134,10 @@ struct file_list_entry *dfu_file_entry; > static int dfufd = -EINVAL; > static struct file_list *dfu_files; > static int dfudetach; > +static struct mtd_info_user dfu_mtdinfo; > +static loff_t dfu_written; > +static loff_t dfu_erased; > +static int prog_erase; These are not initialized, this works for the first file updated via DFU, but what about the following ones? > > /* USB DFU functional descriptor */ > static struct usb_dfu_func_descriptor usb_dfu_func = { > @@ -327,8 +333,16 @@ static void dfu_cleanup(struct f_dfu *dfu) > static void dn_complete(struct usb_ep *ep, struct usb_request *req) > { > struct f_dfu *dfu = req->context; > + loff_t size; > int ret; > > + if (prog_erase && (dfu_written + req->length) > dfu_erased) { > + size = roundup(req->length, dfu_mtdinfo.erasesize); > + erase(dfufd, size, dfu_erased); You should check the return value here. > + dfu_erased += size; > + } > + > + dfu_written += req->length; > ret = write(dfufd, req->buf, req->length); > if (ret < (int)req->length) { > perror("write"); > @@ -493,7 +507,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) > } > > if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) { > - ret = erase(dfufd, ERASE_SIZE_ALL, 0); > + ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo); > + if (ret) /* not a mtd */ > + ret = erase(dfufd, ERASE_SIZE_ALL, 0); > + else > + prog_erase = 1; I am not aware of any non mtd devices that need erase. I think you can drop the full erase here. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd 2020-03-26 6:03 ` Sascha Hauer @ 2020-03-26 9:08 ` Jules Maselbas 0 siblings, 0 replies; 7+ messages in thread From: Jules Maselbas @ 2020-03-26 9:08 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox Hi Sascha, On Thu, Mar 26, 2020 at 07:03:11AM +0100, Sascha Hauer wrote: > Hi Jules, > > > > > @@ -132,6 +134,10 @@ struct file_list_entry *dfu_file_entry; > > static int dfufd = -EINVAL; > > static struct file_list *dfu_files; > > static int dfudetach; > > +static struct mtd_info_user dfu_mtdinfo; > > +static loff_t dfu_written; > > +static loff_t dfu_erased; > > +static int prog_erase; > > These are not initialized, this works for the first file updated via > DFU, but what about the following ones? Good point, this initialization is be required between to file. I don't think cleaning this in unbind will be sufficient, I will try to update multiple files. > > > > > /* USB DFU functional descriptor */ > > static struct usb_dfu_func_descriptor usb_dfu_func = { > > @@ -327,8 +333,16 @@ static void dfu_cleanup(struct f_dfu *dfu) > > static void dn_complete(struct usb_ep *ep, struct usb_request *req) > > { > > struct f_dfu *dfu = req->context; > > + loff_t size; > > int ret; > > > > + if (prog_erase && (dfu_written + req->length) > dfu_erased) { > > + size = roundup(req->length, dfu_mtdinfo.erasesize); > > + erase(dfufd, size, dfu_erased); > > You should check the return value here. Right > > + dfu_erased += size; > > + } > > + > > + dfu_written += req->length; > > ret = write(dfufd, req->buf, req->length); > > if (ret < (int)req->length) { > > perror("write"); > > @@ -493,7 +507,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) > > } > > > > if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) { > > - ret = erase(dfufd, ERASE_SIZE_ALL, 0); > > + ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo); > > + if (ret) /* not a mtd */ > > + ret = erase(dfufd, ERASE_SIZE_ALL, 0); > > + else > > + prog_erase = 1; > > I am not aware of any non mtd devices that need erase. I think you can > drop the full erase here. OK Thanks for the review, Jules _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind 2020-03-24 15:46 [PATCH 0/3] usb: gadget: dfu: Fix timeout on erase when using big partition Jules Maselbas 2020-03-24 15:46 ` [PATCH 1/3] usb: gadget: dfu: Add manifestation phase Jules Maselbas 2020-03-24 15:46 ` [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd Jules Maselbas @ 2020-03-24 15:46 ` Jules Maselbas 2020-03-26 6:09 ` Sascha Hauer 2 siblings, 1 reply; 7+ messages in thread From: Jules Maselbas @ 2020-03-24 15:46 UTC (permalink / raw) To: barebox; +Cc: Jules Maselbas Global variables must be reset to their default value before a new dfu_bind is done. Otherwise things wont work and are likely to cause a system crash due to a use after free: the global dfu_files was still pointing deallocated structure after unbind. Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> --- drivers/usb/gadget/dfu.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c index 592586db1..5504f4933 100644 --- a/drivers/usb/gadget/dfu.c +++ b/drivers/usb/gadget/dfu.c @@ -277,6 +277,15 @@ dfu_unbind(struct usb_configuration *c, struct usb_function *f) { struct f_dfu *dfu = func_to_dfu(f); + memset(&dfu_mtdinfo, 0, sizeof(dfu_mtdinfo)); + dfu_files = NULL; + dfu_file_entry = NULL; + dfufd = -EINVAL; + dfudetach = 0; + dfu_written = 0; + dfu_erased = 0; + prog_erase = 0; + usb_free_all_descriptors(f); dma_free(dfu->dnreq->buf); -- 2.21.0.196.g041f5ea _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind 2020-03-24 15:46 ` [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind Jules Maselbas @ 2020-03-26 6:09 ` Sascha Hauer 0 siblings, 0 replies; 7+ messages in thread From: Sascha Hauer @ 2020-03-26 6:09 UTC (permalink / raw) To: Jules Maselbas; +Cc: barebox On Tue, Mar 24, 2020 at 04:46:47PM +0100, Jules Maselbas wrote: > Global variables must be reset to their default value before a new > dfu_bind is done. Otherwise things wont work and are likely to cause > a system crash due to a use after free: the global dfu_files was still > pointing deallocated structure after unbind. > > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> > --- > drivers/usb/gadget/dfu.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c > index 592586db1..5504f4933 100644 > --- a/drivers/usb/gadget/dfu.c > +++ b/drivers/usb/gadget/dfu.c > @@ -277,6 +277,15 @@ dfu_unbind(struct usb_configuration *c, struct usb_function *f) > { > struct f_dfu *dfu = func_to_dfu(f); > > + memset(&dfu_mtdinfo, 0, sizeof(dfu_mtdinfo)); > + dfu_files = NULL; > + dfu_file_entry = NULL; > + dfufd = -EINVAL; > + dfudetach = 0; > + dfu_written = 0; > + dfu_erased = 0; > + prog_erase = 0; Ah, ok, here is the missing initialization ;) Please swap the order of patches 2/3 and 3/3 Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-26 9:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-24 15:46 [PATCH 0/3] usb: gadget: dfu: Fix timeout on erase when using big partition Jules Maselbas 2020-03-24 15:46 ` [PATCH 1/3] usb: gadget: dfu: Add manifestation phase Jules Maselbas 2020-03-24 15:46 ` [PATCH 2/3] usb: gadget: dfu: Progressive erase if file is a mtd Jules Maselbas 2020-03-26 6:03 ` Sascha Hauer 2020-03-26 9:08 ` Jules Maselbas 2020-03-24 15:46 ` [PATCH 3/3] usb: gadget: dfu: Reset global variables on unbind Jules Maselbas 2020-03-26 6:09 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox