* [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty @ 2019-11-04 16:06 Pete Batard 2019-11-04 16:27 ` Philippe Mathieu-Daudé 2019-11-07 16:21 ` Leif Lindholm 0 siblings, 2 replies; 7+ messages in thread From: Pete Batard @ 2019-11-04 16:06 UTC (permalink / raw) To: devel; +Cc: ard.biesheuvel, leif.lindholm, philmd From: Andrei Warkentin <andrey.warkentin@gmail.com> It is possible for the command line to be empty (Cmd->TagHead.TagValueSize = 0), in which case the code should not attempt to read the value at CommandLine[-1]. Signed-off-by: Pete Batard <pete@akeo.ie> --- Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c index 5a9d4c3f1787..9b4aa068857c 100644 --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { + if (Cmd->TagHead.TagValueSize == 0 || + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { // // Add a NUL terminator if required. // -- 2.21.0.windows.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-04 16:06 [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty Pete Batard @ 2019-11-04 16:27 ` Philippe Mathieu-Daudé 2019-11-07 16:21 ` Leif Lindholm 1 sibling, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2019-11-04 16:27 UTC (permalink / raw) To: Pete Batard, devel; +Cc: ard.biesheuvel, leif.lindholm On 11/4/19 5:06 PM, Pete Batard wrote: > From: Andrei Warkentin <andrey.warkentin@gmail.com> > > It is possible for the command line to be empty > (Cmd->TagHead.TagValueSize = 0), in which case the code should not > attempt to read the value at CommandLine[-1]. Oops... Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > Signed-off-by: Pete Batard <pete@akeo.ie> > --- > Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > index 5a9d4c3f1787..9b4aa068857c 100644 > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( > > CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); > > - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > + if (Cmd->TagHead.TagValueSize == 0 || > + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > // > // Add a NUL terminator if required. > // > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-04 16:06 [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty Pete Batard 2019-11-04 16:27 ` Philippe Mathieu-Daudé @ 2019-11-07 16:21 ` Leif Lindholm 2019-11-07 17:05 ` Pete Batard 1 sibling, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2019-11-07 16:21 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel, philmd Patch looks good, but the term "command line" is a bit confusing. I assume we're talking about whatever way parameters are passed from pre-edk2 firmware to edk2, right? Is there a more precise term for this? / Leif On Mon, Nov 04, 2019 at 04:06:17PM +0000, Pete Batard wrote: > From: Andrei Warkentin <andrey.warkentin@gmail.com> > > It is possible for the command line to be empty > (Cmd->TagHead.TagValueSize = 0), in which case the code should not > attempt to read the value at CommandLine[-1]. > > Signed-off-by: Pete Batard <pete@akeo.ie> > --- > Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > index 5a9d4c3f1787..9b4aa068857c 100644 > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( > > CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); > > - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > + if (Cmd->TagHead.TagValueSize == 0 || > + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > // > // Add a NUL terminator if required. > // > -- > 2.21.0.windows.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-07 16:21 ` Leif Lindholm @ 2019-11-07 17:05 ` Pete Batard 2019-11-07 17:27 ` Leif Lindholm 0 siblings, 1 reply; 7+ messages in thread From: Pete Batard @ 2019-11-07 17:05 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel, philmd Hi Leif, On 2019.11.07 16:21, Leif Lindholm wrote: > Patch looks good, but the term "command line" is a bit confusing. > > I assume we're talking about whatever way parameters are passed from > pre-edk2 firmware to edk2, right? Yes. This is basically what the Raspberry Pi VideoCore bootcode digests and passes as boot arguments to the ARM boot loader (i.e. our TF-A + EFI firmware executable). It contains options that the user may have set in their 'config.txt' as well as other data. > Is there a more precise term for this? Would "boot arguments" or "external boot arguments" work for you? Or if you prefer "(external) boot parameters" should be applicable too. Regards, /Pete > > / > Leif > > On Mon, Nov 04, 2019 at 04:06:17PM +0000, Pete Batard wrote: >> From: Andrei Warkentin <andrey.warkentin@gmail.com> >> >> It is possible for the command line to be empty >> (Cmd->TagHead.TagValueSize = 0), in which case the code should not >> attempt to read the value at CommandLine[-1]. >> >> Signed-off-by: Pete Batard <pete@akeo.ie> >> --- >> Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> index 5a9d4c3f1787..9b4aa068857c 100644 >> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >> @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( >> >> CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); >> >> - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { >> + if (Cmd->TagHead.TagValueSize == 0 || >> + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { >> // >> // Add a NUL terminator if required. >> // >> -- >> 2.21.0.windows.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-07 17:05 ` Pete Batard @ 2019-11-07 17:27 ` Leif Lindholm 2019-11-07 17:35 ` Pete Batard 0 siblings, 1 reply; 7+ messages in thread From: Leif Lindholm @ 2019-11-07 17:27 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel, philmd On Thu, Nov 07, 2019 at 05:05:20PM +0000, Pete Batard wrote: > Hi Leif, > > On 2019.11.07 16:21, Leif Lindholm wrote: > > Patch looks good, but the term "command line" is a bit confusing. > > > > I assume we're talking about whatever way parameters are passed from > > pre-edk2 firmware to edk2, right? > > Yes. This is basically what the Raspberry Pi VideoCore bootcode digests and > passes as boot arguments to the ARM boot loader (i.e. our TF-A + EFI > firmware executable). It contains options that the user may have set in > their 'config.txt' as well as other data. Sure. > > Is there a more precise term for this? > > Would "boot arguments" or "external boot arguments" work for you? Or if you > prefer "(external) boot parameters" should be applicable too. Either would be fine - I was just hoping there might be a recognized standard name for them :) So, I could update the subject line to Platform/RPi: Prevent external boot arguments over-read in order to keep it short, and change "command line" in the commit message body to "external boot arguments" - does that work for you? Regards, Leif > Regards, > > /Pete > > > > > / > > Leif > > > > On Mon, Nov 04, 2019 at 04:06:17PM +0000, Pete Batard wrote: > > > From: Andrei Warkentin <andrey.warkentin@gmail.com> > > > > > > It is possible for the command line to be empty > > > (Cmd->TagHead.TagValueSize = 0), in which case the code should not > > > attempt to read the value at CommandLine[-1]. > > > > > > Signed-off-by: Pete Batard <pete@akeo.ie> > > > --- > > > Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > index 5a9d4c3f1787..9b4aa068857c 100644 > > > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( > > > CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); > > > - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > > > + if (Cmd->TagHead.TagValueSize == 0 || > > > + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > > > // > > > // Add a NUL terminator if required. > > > // > > > -- > > > 2.21.0.windows.1 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-07 17:27 ` Leif Lindholm @ 2019-11-07 17:35 ` Pete Batard 2019-11-07 18:09 ` Leif Lindholm 0 siblings, 1 reply; 7+ messages in thread From: Pete Batard @ 2019-11-07 17:35 UTC (permalink / raw) To: Leif Lindholm; +Cc: devel, ard.biesheuvel, philmd On 2019.11.07 17:27, Leif Lindholm wrote: > On Thu, Nov 07, 2019 at 05:05:20PM +0000, Pete Batard wrote: >> Hi Leif, >> >> On 2019.11.07 16:21, Leif Lindholm wrote: >>> Patch looks good, but the term "command line" is a bit confusing. >>> >>> I assume we're talking about whatever way parameters are passed from >>> pre-edk2 firmware to edk2, right? >> >> Yes. This is basically what the Raspberry Pi VideoCore bootcode digests and >> passes as boot arguments to the ARM boot loader (i.e. our TF-A + EFI >> firmware executable). It contains options that the user may have set in >> their 'config.txt' as well as other data. > > Sure. > >>> Is there a more precise term for this? >> >> Would "boot arguments" or "external boot arguments" work for you? Or if you >> prefer "(external) boot parameters" should be applicable too. > > Either would be fine - I was just hoping there might be a recognized > standard name for them :) Well, as far as I know, the recognized standard name is "commandline" as per https://www.raspberrypi.org/documentation/configuration/config-txt/boot.md For instance there exists an option called disable_commandline_tags which pertains to what we are talking about. > So, I could update the subject line to > Platform/RPi: Prevent external boot arguments over-read > in order to keep it short, and change "command line" in the commit > message body to "external boot arguments" - does that work for you? If you can do that, that's great. Thanks! /Pete > > Regards, > > Leif > >> Regards, >> >> /Pete >> >>> >>> / >>> Leif >>> >>> On Mon, Nov 04, 2019 at 04:06:17PM +0000, Pete Batard wrote: >>>> From: Andrei Warkentin <andrey.warkentin@gmail.com> >>>> >>>> It is possible for the command line to be empty >>>> (Cmd->TagHead.TagValueSize = 0), in which case the code should not >>>> attempt to read the value at CommandLine[-1]. >>>> >>>> Signed-off-by: Pete Batard <pete@akeo.ie> >>>> --- >>>> Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> index 5a9d4c3f1787..9b4aa068857c 100644 >>>> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( >>>> CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); >>>> - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { >>>> + if (Cmd->TagHead.TagValueSize == 0 || >>>> + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { >>>> // >>>> // Add a NUL terminator if required. >>>> // >>>> -- >>>> 2.21.0.windows.1 >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty 2019-11-07 17:35 ` Pete Batard @ 2019-11-07 18:09 ` Leif Lindholm 0 siblings, 0 replies; 7+ messages in thread From: Leif Lindholm @ 2019-11-07 18:09 UTC (permalink / raw) To: Pete Batard; +Cc: devel, ard.biesheuvel, philmd On Thu, Nov 07, 2019 at 05:35:27PM +0000, Pete Batard wrote: > On 2019.11.07 17:27, Leif Lindholm wrote: > > On Thu, Nov 07, 2019 at 05:05:20PM +0000, Pete Batard wrote: > > > Hi Leif, > > > > > > On 2019.11.07 16:21, Leif Lindholm wrote: > > > > Patch looks good, but the term "command line" is a bit confusing. > > > > > > > > I assume we're talking about whatever way parameters are passed from > > > > pre-edk2 firmware to edk2, right? > > > > > > Yes. This is basically what the Raspberry Pi VideoCore bootcode digests and > > > passes as boot arguments to the ARM boot loader (i.e. our TF-A + EFI > > > firmware executable). It contains options that the user may have set in > > > their 'config.txt' as well as other data. > > > > Sure. > > > > > > Is there a more precise term for this? > > > > > > Would "boot arguments" or "external boot arguments" work for you? Or if you > > > prefer "(external) boot parameters" should be applicable too. > > > > Either would be fine - I was just hoping there might be a recognized > > standard name for them :) > > Well, as far as I know, the recognized standard name is "commandline" as per > https://www.raspberrypi.org/documentation/configuration/config-txt/boot.md > > For instance there exists an option called disable_commandline_tags which > pertains to what we are talking about. Right. I meant something non-context-dependent. Nevermind :) > > So, I could update the subject line to > > Platform/RPi: Prevent external boot arguments over-read > > in order to keep it short, and change "command line" in the commit > > message body to "external boot arguments" - does that work for you? > > If you can do that, that's great. Thanks! Done. And with that: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> Pushed as 92f06ccddfcf. Thanks! / Leif > /Pete > > > > > Regards, > > > > Leif > > > > > Regards, > > > > > > /Pete > > > > > > > > > > > / > > > > Leif > > > > > > > > On Mon, Nov 04, 2019 at 04:06:17PM +0000, Pete Batard wrote: > > > > > From: Andrei Warkentin <andrey.warkentin@gmail.com> > > > > > > > > > > It is possible for the command line to be empty > > > > > (Cmd->TagHead.TagValueSize = 0), in which case the code should not > > > > > attempt to read the value at CommandLine[-1]. > > > > > > > > > > Signed-off-by: Pete Batard <pete@akeo.ie> > > > > > --- > > > > > Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > > > index 5a9d4c3f1787..9b4aa068857c 100644 > > > > > --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > > > +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c > > > > > @@ -927,7 +927,8 @@ RpiFirmwareGetCommmandLine ( > > > > > CopyMem (CommandLine, Cmd->CommandLine, Cmd->TagHead.TagValueSize); > > > > > - if (CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > > > > > + if (Cmd->TagHead.TagValueSize == 0 || > > > > > + CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') { > > > > > // > > > > > // Add a NUL terminator if required. > > > > > // > > > > > -- > > > > > 2.21.0.windows.1 > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-07 18:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-04 16:06 [edk2-platforms][PATCH 1/1] Platform/RPi: Prevent buffer over-read when the command line is empty Pete Batard 2019-11-04 16:27 ` Philippe Mathieu-Daudé 2019-11-07 16:21 ` Leif Lindholm 2019-11-07 17:05 ` Pete Batard 2019-11-07 17:27 ` Leif Lindholm 2019-11-07 17:35 ` Pete Batard 2019-11-07 18:09 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox