| Message ID | 29e817be984471dc2438a9414a9a7e1768d62950.1626169090.git.sean@mess.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix various issues with RFD and FTLs | expand |
Hi Sean, Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100: > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use > container_of() to make it clearer what the cast is doing. > > Signed-off-by: Sean Young <sean@mess.org> > --- > drivers/mtd/rfd_ftl.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c > index 7f5f6d247cae..af20a0a71108 100644 > --- a/drivers/mtd/rfd_ftl.c > +++ b/drivers/mtd/rfd_ftl.c [...] > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev) > part->mbd.mtd->name, i, part->blocks[i].erases); > } > > - del_mtd_blktrans_dev(dev); > vfree(part->sector_map); > kfree(part->header_cache); > kfree(part->blocks); > + del_mtd_blktrans_dev(&part->mbd); I am not sure moving this call at the bottom of ftl_remove_dev makes sense, can we keep it where it was and just do the s/dev/part->mbd/ ? > } > > static struct mtd_blktrans_ops rfd_ftl_tr = { Thanks, Miquèl
On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote: > Hi Sean, > > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100: > > > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use > > container_of() to make it clearer what the cast is doing. > > > > Signed-off-by: Sean Young <sean@mess.org> > > --- > > drivers/mtd/rfd_ftl.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c > > index 7f5f6d247cae..af20a0a71108 100644 > > --- a/drivers/mtd/rfd_ftl.c > > +++ b/drivers/mtd/rfd_ftl.c > > [...] > > > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct > mtd_blktrans_dev *dev) > > part->mbd.mtd->name, i, part->blocks[i].erases); > > } > > > > - del_mtd_blktrans_dev(dev); > > vfree(part->sector_map); > > kfree(part->header_cache); > > kfree(part->blocks); > > + del_mtd_blktrans_dev(&part->mbd); > > I am not sure moving this call at the bottom of ftl_remove_dev makes > sense, can we keep it where it was and just do the s/dev/part->mbd/ ? The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument, so both part and dev point to freed memory. This means it's a use after free. Thanks, Sean > > > } > > > > static struct mtd_blktrans_ops rfd_ftl_tr = { > > Thanks, > Miquèl > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Sean, Sean Young <sean@mess.org> wrote on Sat, 7 Aug 2021 08:57:35 +0100: > On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote: > > Hi Sean, > > > > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100: > > > > > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use > > > container_of() to make it clearer what the cast is doing. > > > > > > Signed-off-by: Sean Young <sean@mess.org> > > > --- > > > drivers/mtd/rfd_ftl.c | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c > > > index 7f5f6d247cae..af20a0a71108 100644 > > > --- a/drivers/mtd/rfd_ftl.c > > > +++ b/drivers/mtd/rfd_ftl.c > > > > [...] > > > > > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct > > mtd_blktrans_dev *dev) > > > part->mbd.mtd->name, i, part->blocks[i].erases); > > > } > > > > > > - del_mtd_blktrans_dev(dev); > > > vfree(part->sector_map); > > > kfree(part->header_cache); > > > kfree(part->blocks); > > > + del_mtd_blktrans_dev(&part->mbd); > > > > I am not sure moving this call at the bottom of ftl_remove_dev makes > > sense, can we keep it where it was and just do the s/dev/part->mbd/ ? > > The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument, > so both part and dev point to freed memory. This means it's a use after free. Ok, please split this into two patches and we'll be good. Thanks, Miquèl
Hi Miquel, On Sat, Aug 07, 2021 at 12:34:09PM +0200, Miquel Raynal wrote: > Hi Sean, > > Sean Young <sean@mess.org> wrote on Sat, 7 Aug 2021 08:57:35 +0100: > > > On Fri, Aug 06, 2021 at 08:21:58PM +0200, Miquel Raynal wrote: > > > Hi Sean, > > > > > > Sean Young <sean@mess.org> wrote on Tue, 13 Jul 2021 10:44:03 +0100: > > > > > > > del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use > > > > container_of() to make it clearer what the cast is doing. > > > > > > > > Signed-off-by: Sean Young <sean@mess.org> > > > > --- > > > > drivers/mtd/rfd_ftl.c | 14 +++++++------- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c > > > > index 7f5f6d247cae..af20a0a71108 100644 > > > > --- a/drivers/mtd/rfd_ftl.c > > > > +++ b/drivers/mtd/rfd_ftl.c > > > > > > [...] > > > > > > > @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct > > > mtd_blktrans_dev *dev) > > > > part->mbd.mtd->name, i, part->blocks[i].erases); > > > > } > > > > > > > > - del_mtd_blktrans_dev(dev); > > > > vfree(part->sector_map); > > > > kfree(part->header_cache); > > > > kfree(part->blocks); > > > > + del_mtd_blktrans_dev(&part->mbd); > > > > > > I am not sure moving this call at the bottom of ftl_remove_dev makes > > > sense, can we keep it where it was and just do the s/dev/part->mbd/ ? > > > > The reason for this patch is that del_mtd_blktrans_dev() kfrees its argument, > > so both part and dev point to freed memory. This means it's a use after free. > > Ok, please split this into two patches and we'll be good. Good point. Thank you for the review. I'll send out v2 shortly. Sean
diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c index 7f5f6d247cae..af20a0a71108 100644 --- a/drivers/mtd/rfd_ftl.c +++ b/drivers/mtd/rfd_ftl.c @@ -239,7 +239,7 @@ static int scan_header(struct partition *part) static int rfd_ftl_readsect(struct mtd_blktrans_dev *dev, u_long sector, char *buf) { - struct partition *part = (struct partition*)dev; + struct partition *part = container_of(dev, struct partition, mbd); u_long addr; size_t retlen; int rc; @@ -600,7 +600,7 @@ static int find_free_sector(const struct partition *part, const struct block *bl static int do_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf, ulong *old_addr) { - struct partition *part = (struct partition*)dev; + struct partition *part = container_of(dev, struct partition, mbd); struct block *block; u_long addr; int i; @@ -666,7 +666,7 @@ static int do_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf, static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char *buf) { - struct partition *part = (struct partition*)dev; + struct partition *part = container_of(dev, struct partition, mbd); u_long old_addr; int i; int rc = 0; @@ -708,7 +708,7 @@ static int rfd_ftl_writesect(struct mtd_blktrans_dev *dev, u_long sector, char * static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev, unsigned long sector, unsigned int nr_sects) { - struct partition *part = (struct partition *)dev; + struct partition *part = container_of(dev, struct partition, mbd); u_long addr; int rc; @@ -735,7 +735,7 @@ static int rfd_ftl_discardsect(struct mtd_blktrans_dev *dev, static int rfd_ftl_getgeo(struct mtd_blktrans_dev *dev, struct hd_geometry *geo) { - struct partition *part = (struct partition*)dev; + struct partition *part = container_of(dev, struct partition, mbd); geo->heads = 1; geo->sectors = SECTORS_PER_TRACK; @@ -792,7 +792,7 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd) static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev) { - struct partition *part = (struct partition*)dev; + struct partition *part = container_of(dev, struct partition, mbd); int i; for (i=0; i<part->total_blocks; i++) { @@ -800,10 +800,10 @@ static void rfd_ftl_remove_dev(struct mtd_blktrans_dev *dev) part->mbd.mtd->name, i, part->blocks[i].erases); } - del_mtd_blktrans_dev(dev); vfree(part->sector_map); kfree(part->header_cache); kfree(part->blocks); + del_mtd_blktrans_dev(&part->mbd); } static struct mtd_blktrans_ops rfd_ftl_tr = {
del_mtd_blktrans_dev() will kfree part, so this is a use-after-free. Use container_of() to make it clearer what the cast is doing. Signed-off-by: Sean Young <sean@mess.org> --- drivers/mtd/rfd_ftl.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)