| Message ID | 20211010215241.872206-1-marek.vasut@gmail.com |
|---|---|
| State | Accepted |
| Commit | c6855195e4b4dd07d1ae04d9d98ed999f65b7dc3 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | loads: Block writes into LMB reserved areas of U-Boot | expand |
Hi Marek, On Sun, 10 Oct 2021 at 15:52, <marek.vasut@gmail.com> wrote: > > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The loads srec loading may overwrite piece of U-Boot accidentally. > Prevent that by using LMB to detect whether upcoming write would > overwrite piece of reserved U-Boot code, and if that is the case, > abort the srec loading. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > cmd/load.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/cmd/load.c b/cmd/load.c > index 249ebd4ae0..7e4a552d90 100644 > --- a/cmd/load.c > +++ b/cmd/load.c > @@ -16,6 +16,7 @@ > #include <exports.h> > #include <flash.h> > #include <image.h> > +#include <lmb.h> > #include <mapmem.h> > #include <net.h> > #include <s_record.h> > @@ -137,6 +138,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc, > > static ulong load_serial(long offset) > { > + struct lmb lmb; > char record[SREC_MAXRECLEN + 1]; /* buffer for one S-Record */ > char binbuf[SREC_MAXBINLEN]; /* buffer for binary data */ > int binlen; /* no. of data bytes in S-Rec. */ > @@ -147,6 +149,9 @@ static ulong load_serial(long offset) > ulong start_addr = ~0; > ulong end_addr = 0; > int line_count = 0; > + long ret; > + > + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); > > while (read_record(record, SREC_MAXRECLEN + 1) >= 0) { > type = srec_decode(record, &binlen, &addr, binbuf); > @@ -172,7 +177,14 @@ static ulong load_serial(long offset) > } else > #endif > { > + ret = lmb_reserve(&lmb, store_addr, binlen); > + if (ret) { > + printf("\nCannot overwrite reserved area (%08lx..%08lx)\n", > + store_addr, store_addr + binlen); > + return ret; > + } > memcpy((char *)(store_addr), binbuf, binlen); > + lmb_free(&lmb, store_addr, binlen); > } > if ((store_addr) < start_addr) > start_addr = store_addr; > -- > 2.33.0 > Reviewed-by: Simon Glass <sjg@chromium.org> This code looks OK but I don't know what lmb_reserve() and lmb_free() do. Can you add comments to the header file? Regards, Simon
On 10/14/21 5:10 PM, Simon Glass wrote: [...] >> @@ -137,6 +138,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc, >> >> static ulong load_serial(long offset) >> { >> + struct lmb lmb; >> char record[SREC_MAXRECLEN + 1]; /* buffer for one S-Record */ >> char binbuf[SREC_MAXBINLEN]; /* buffer for binary data */ >> int binlen; /* no. of data bytes in S-Rec. */ >> @@ -147,6 +149,9 @@ static ulong load_serial(long offset) >> ulong start_addr = ~0; >> ulong end_addr = 0; >> int line_count = 0; >> + long ret; >> + >> + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); >> >> while (read_record(record, SREC_MAXRECLEN + 1) >= 0) { >> type = srec_decode(record, &binlen, &addr, binbuf); >> @@ -172,7 +177,14 @@ static ulong load_serial(long offset) >> } else >> #endif >> { >> + ret = lmb_reserve(&lmb, store_addr, binlen); >> + if (ret) { >> + printf("\nCannot overwrite reserved area (%08lx..%08lx)\n", >> + store_addr, store_addr + binlen); >> + return ret; >> + } >> memcpy((char *)(store_addr), binbuf, binlen); >> + lmb_free(&lmb, store_addr, binlen); >> } >> if ((store_addr) < start_addr) >> start_addr = store_addr; >> -- >> 2.33.0 >> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > This code looks OK but I don't know what lmb_reserve() and lmb_free() > do. Can you add comments to the header file? Not here, the entire LMB stuff needs (better) documentation, that's where (all) such clarification should go.
On Fri, Oct 15, 2021 at 04:23:27PM +0200, Marek Vasut wrote: > On 10/14/21 5:10 PM, Simon Glass wrote: > [...] > > > @@ -137,6 +138,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > > static ulong load_serial(long offset) > > > { > > > + struct lmb lmb; > > > char record[SREC_MAXRECLEN + 1]; /* buffer for one S-Record */ > > > char binbuf[SREC_MAXBINLEN]; /* buffer for binary data */ > > > int binlen; /* no. of data bytes in S-Rec. */ > > > @@ -147,6 +149,9 @@ static ulong load_serial(long offset) > > > ulong start_addr = ~0; > > > ulong end_addr = 0; > > > int line_count = 0; > > > + long ret; > > > + > > > + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); > > > > > > while (read_record(record, SREC_MAXRECLEN + 1) >= 0) { > > > type = srec_decode(record, &binlen, &addr, binbuf); > > > @@ -172,7 +177,14 @@ static ulong load_serial(long offset) > > > } else > > > #endif > > > { > > > + ret = lmb_reserve(&lmb, store_addr, binlen); > > > + if (ret) { > > > + printf("\nCannot overwrite reserved area (%08lx..%08lx)\n", > > > + store_addr, store_addr + binlen); > > > + return ret; > > > + } > > > memcpy((char *)(store_addr), binbuf, binlen); > > > + lmb_free(&lmb, store_addr, binlen); > > > } > > > if ((store_addr) < start_addr) > > > start_addr = store_addr; > > > -- > > > 2.33.0 > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > This code looks OK but I don't know what lmb_reserve() and lmb_free() > > do. Can you add comments to the header file? > > Not here, the entire LMB stuff needs (better) documentation, that's where > (all) such clarification should go. Is that you saying you'll do such a follow-up patch, given you've touched this code the most of late?
On 10/15/21 6:09 PM, Tom Rini wrote: [...] >>> This code looks OK but I don't know what lmb_reserve() and lmb_free() >>> do. Can you add comments to the header file? >> >> Not here, the entire LMB stuff needs (better) documentation, that's where >> (all) such clarification should go. > > Is that you saying you'll do such a follow-up patch, given you've > touched this code the most of late? Maybe, I won't promise you anything except I add it to my roll of todo paper.
On Sun, Oct 10, 2021 at 11:52:41PM +0200, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > The loads srec loading may overwrite piece of U-Boot accidentally. > Prevent that by using LMB to detect whether upcoming write would > overwrite piece of reserved U-Boot code, and if that is the case, > abort the srec loading. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
diff --git a/cmd/load.c b/cmd/load.c index 249ebd4ae0..7e4a552d90 100644 --- a/cmd/load.c +++ b/cmd/load.c @@ -16,6 +16,7 @@ #include <exports.h> #include <flash.h> #include <image.h> +#include <lmb.h> #include <mapmem.h> #include <net.h> #include <s_record.h> @@ -137,6 +138,7 @@ static int do_load_serial(struct cmd_tbl *cmdtp, int flag, int argc, static ulong load_serial(long offset) { + struct lmb lmb; char record[SREC_MAXRECLEN + 1]; /* buffer for one S-Record */ char binbuf[SREC_MAXBINLEN]; /* buffer for binary data */ int binlen; /* no. of data bytes in S-Rec. */ @@ -147,6 +149,9 @@ static ulong load_serial(long offset) ulong start_addr = ~0; ulong end_addr = 0; int line_count = 0; + long ret; + + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); while (read_record(record, SREC_MAXRECLEN + 1) >= 0) { type = srec_decode(record, &binlen, &addr, binbuf); @@ -172,7 +177,14 @@ static ulong load_serial(long offset) } else #endif { + ret = lmb_reserve(&lmb, store_addr, binlen); + if (ret) { + printf("\nCannot overwrite reserved area (%08lx..%08lx)\n", + store_addr, store_addr + binlen); + return ret; + } memcpy((char *)(store_addr), binbuf, binlen); + lmb_free(&lmb, store_addr, binlen); } if ((store_addr) < start_addr) start_addr = store_addr;