Re: [PATCH v2 0/3] Cryptomount detached headers
On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote: > Note: Description here has changed since last time, the last few sentences, > which describe a method for allowing backends to bypass the read hook on > the source disk. > > Updates since v1: > * Add marge comment block describing at a high level the read hook operation >and assumptions. > * Add code to unset read hook on source disk when exiting >grub_cryptodisk_scan_device_real(). A couple return calls were converted >into gotos so that logical flows to one exit point. > > This patch series is, I believe, a better approach to supporting detached > headers for cryptomount and backends. This series will probably not apply > cleanly without the changes from the recent series entitled "[PATCH 0/4] > Cryptomount keyfile support". But its only because they touch code in the > same vicinity, not because there's any real dependency. Does not this series require rebase on master after merging "Cryptomount keyfile support" patch set? If yes I will review these patches after rebase. I hope it will be quick turn around. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] tests: Show host determined fs UUID when hfs UUID test fails
On Wed, May 11, 2022 at 10:19:44PM -0500, Glenn Washburn wrote: > On failure, the hfs test should show both the host and GRUB determined fs > UUID. Prior to this change, both outputs where generated by GRUB, which is > less helpful in determining the cause of failure. > > Signed-off-by: Glenn Washburn > --- > tests/util/grub-fs-tester.in | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in > index 3c1d712de..d323665d2 100644 > --- a/tests/util/grub-fs-tester.in > +++ b/tests/util/grub-fs-tester.in > @@ -1353,7 +1353,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" > "$MAXLOGSECSIZE" 1); do > : > else > echo UUID FAIL > - echo "$LSOUT" > + echo "$FSUUID" I am not sure why you drop 'echo "$LSOUT"' here. Otherwise patches LGTM. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH v3 5/5] Cleanup: grub-mkconfig_lib: remove unused version comparison functions
Michael Chang via Grub-devel writes: > On Thu, May 26, 2022 at 05:07:11PM -0400, Robbie Harwood wrote: >> Mathieu Desnoyers writes: >> >>> There are no users left of version_find_latest(), version_test_gt(), >>> and version_test_numeric(). Remove those unused helper >>> functions. Using those helper functions is what caused the quadratic >>> sorting performance issues in the first place, so removing them is a >>> net win. >>> >>> Signed-off-by: Mathieu Desnoyers >> >> Reviewed-by: Robbie Harwood > > Hm. This seems to contradict your proposed patch to use distro specific > sort by hooking into those functions got removed here. > > mkconfig: use distro sorts when available > https://www.mail-archive.com/grub-devel@gnu.org/msg33357.html > > I'd like to know more your comments about this as those hooks might > still be needed or where to keep distribution's sort ? The series does, yes - both can't be applied as-is. I'm fine to rebase mine, but haven't done it yet. I don't mind adding them back if I need to. Be well, --Robbie signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] tests: Show host determined fs UUID when hfs UUID test fails
On Fri, 27 May 2022 16:16:48 +0200 Daniel Kiper wrote: > On Wed, May 11, 2022 at 10:19:44PM -0500, Glenn Washburn wrote: > > On failure, the hfs test should show both the host and GRUB determined fs > > UUID. Prior to this change, both outputs where generated by GRUB, which is > > less helpful in determining the cause of failure. > > > > Signed-off-by: Glenn Washburn > > --- > > tests/util/grub-fs-tester.in | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in > > index 3c1d712de..d323665d2 100644 > > --- a/tests/util/grub-fs-tester.in > > +++ b/tests/util/grub-fs-tester.in > > @@ -1353,7 +1353,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" > > "$MAXLOGSECSIZE" 1); do > > : > > else > > echo UUID FAIL > > - echo "$LSOUT" > > + echo "$FSUUID" > > I am not sure why you drop 'echo "$LSOUT"' here. I dropped it because I didn't see the utility in keeping it. Perhaps there is though, and I'm generally in favor of more output on error than less. I'm not opposed to keeping it in. If you're like to keep that line, do you want me to spin a new patch series or can you handle it in the merge? Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/3] Cryptomount detached headers
On Fri, 27 May 2022 16:06:42 +0200 Daniel Kiper wrote: > On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote: > > Note: Description here has changed since last time, the last few sentences, > > which describe a method for allowing backends to bypass the read hook on > > the source disk. > > > > Updates since v1: > > * Add marge comment block describing at a high level the read hook > > operation > >and assumptions. > > * Add code to unset read hook on source disk when exiting > >grub_cryptodisk_scan_device_real(). A couple return calls were converted > >into gotos so that logical flows to one exit point. > > > > This patch series is, I believe, a better approach to supporting detached > > headers for cryptomount and backends. This series will probably not apply > > cleanly without the changes from the recent series entitled "[PATCH 0/4] > > Cryptomount keyfile support". But its only because they touch code in the > > same vicinity, not because there's any real dependency. > > Does not this series require rebase on master after merging "Cryptomount > keyfile support" patch set? If yes I will review these patches after > rebase. I hope it will be quick turn around. The keyfile changes haven't hit master yet, so I can't be sure. But, I think this should apply cleanly. Randgediff shows no difference between the patches in the branch for this series and the branch for the latest keyfile series. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 0/3] Cryptomount detached headers
On Fri, May 27, 2022 at 03:20:26PM -0500, Glenn Washburn wrote: > On Fri, 27 May 2022 16:06:42 +0200 > Daniel Kiper wrote: > > On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote: > > > Note: Description here has changed since last time, the last few > > > sentences, > > > which describe a method for allowing backends to bypass the read hook on > > > the source disk. > > > > > > Updates since v1: > > > * Add marge comment block describing at a high level the read hook > > > operation > > >and assumptions. > > > * Add code to unset read hook on source disk when exiting > > >grub_cryptodisk_scan_device_real(). A couple return calls were > > > converted > > >into gotos so that logical flows to one exit point. > > > > > > This patch series is, I believe, a better approach to supporting detached > > > headers for cryptomount and backends. This series will probably not apply > > > cleanly without the changes from the recent series entitled "[PATCH 0/4] > > > Cryptomount keyfile support". But its only because they touch code in the > > > same vicinity, not because there's any real dependency. > > > > Does not this series require rebase on master after merging "Cryptomount > > keyfile support" patch set? If yes I will review these patches after > > rebase. I hope it will be quick turn around. > > The keyfile changes haven't hit master yet, so I can't be sure. But, I > think this should apply cleanly. Randgediff shows no difference between > the patches in the branch for this series and the branch for the latest > keyfile series. If there are no difference I will review these patches. Thanks, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] tests: Show host determined fs UUID when hfs UUID test fails
On Fri, May 27, 2022 at 01:39:25PM -0500, Glenn Washburn wrote: > On Fri, 27 May 2022 16:16:48 +0200 > Daniel Kiper wrote: > > > On Wed, May 11, 2022 at 10:19:44PM -0500, Glenn Washburn wrote: > > > On failure, the hfs test should show both the host and GRUB determined fs > > > UUID. Prior to this change, both outputs where generated by GRUB, which is > > > less helpful in determining the cause of failure. > > > > > > Signed-off-by: Glenn Washburn > > > --- > > > tests/util/grub-fs-tester.in | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in > > > index 3c1d712de..d323665d2 100644 > > > --- a/tests/util/grub-fs-tester.in > > > +++ b/tests/util/grub-fs-tester.in > > > @@ -1353,7 +1353,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" > > > "$MAXLOGSECSIZE" 1); do > > > : > > > else > > > echo UUID FAIL > > > - echo "$LSOUT" > > > + echo "$FSUUID" > > > > I am not sure why you drop 'echo "$LSOUT"' here. > > I dropped it because I didn't see the utility in keeping it. Perhaps > there is though, and I'm generally in favor of more output on error > than less. I'm not opposed to keeping it in. If you're like to keep I prefer to leave it to be in par with other messages in the file. > that line, do you want me to spin a new patch series or can you handle > it in the merge? I will fix it myself before the push. Thanks, Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC PATCH v3 5/5] Cleanup: grub-mkconfig_lib: remove unused version comparison functions
On Fri, May 27, 2022 at 10:56:38AM -0400, Robbie Harwood wrote: > Michael Chang via Grub-devel writes: > > > On Thu, May 26, 2022 at 05:07:11PM -0400, Robbie Harwood wrote: > >> Mathieu Desnoyers writes: > >> > >>> There are no users left of version_find_latest(), version_test_gt(), > >>> and version_test_numeric(). Remove those unused helper > >>> functions. Using those helper functions is what caused the quadratic > >>> sorting performance issues in the first place, so removing them is a > >>> net win. > >>> > >>> Signed-off-by: Mathieu Desnoyers > >> > >> Reviewed-by: Robbie Harwood > > > > Hm. This seems to contradict your proposed patch to use distro specific > > sort by hooking into those functions got removed here. > > > > mkconfig: use distro sorts when available > > https://www.mail-archive.com/grub-devel@gnu.org/msg33357.html > > > > I'd like to know more your comments about this as those hooks might > > still be needed or where to keep distribution's sort ? > > The series does, yes - both can't be applied as-is. I'm fine to rebase > mine, but haven't done it yet. I don't mind adding them back if I need > to. I prefer to drop unused code now and add it back when it is needed. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/6] Fix coverity bugs and add checks for elf values in grub-core
On Thu, May 26, 2022 at 03:29:46PM -0400, Alec Brown wrote: > Coverity identified several untrusted loop bounds and untrusted allocation > size > bugs in grub-core/loader/i386/bsdXX.c and grub-core/loader/multiboot_elfXX.c. > Upon review of these bugs, I found that specific checks weren't being made to > various elf header values based on the elf manual page. The first four patches > in this patch series address the coverity bugs, as well as adds functions to > check for the correct elf header values. The last two patches adds fixes to > previous work done in util/grub-module-verifierXX.c that also relates to > making > checks of elf header values. > > The Coverity bugs being addressed are: > CID 314018 > CID 314030 > CID 314031 > CID 314039 > > Alec Brown (6): > grub-core/loader/i386/bsdXX.c: Avoid downcasting (char *) to (Elf_Shdr > *) > elf: Validate number of elf section header table entries > elf: Validate elf section header table index for section name string > table > elf: Validate number of elf program header table entries > util/grub-module-verifierXX.c: Add e_shoff check in get_shdr() > util/grub-module-verifierXX.c: Changed get_shnum() return type For all patches Reviewed-by: Daniel Kiper . Thank you for fixing these issues! Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
