Re: [PATCH] efi: strip off final NULL from File Path in grub_efi_get_filename

[Andrei Borzenkov]

25.02.2017 02:21, Vladimir 'phcoder' Serbinenko пишет:
> On Fri, Feb 24, 2017, 09:47 Andrei Borzenkov  wrote:
> 
>> UEFI 2.6 9.3.6.4 File Path Media Device Path says that Path Name is
>> "A NULL-terminated Path string including directory and file names".
>>
>> Strip final NULL from Path Name in each File Path node when constructing
>> full path. To be on safe side, strip all of them.
>>
>> Fixes failure chainloading grub from grub, when loaded grub truncates
>> image path and does not find its grub.cfg.
>>
>> https://bugzilla.opensuse.org/show_bug.cgi?id=1026344
>>
>> This was triggered by commit ce95549cc54b5d6f494608a7c390dba3aab4fba7;
>> before it we built Path Name without trailing NULL, and apparently all
>> other bootloaders use single File Path node, thus not exposing this bug.
>>
>> ---
>> @Vladimir, @Daniel - this fixes regression in rc1 and is real fix for
>> the mentioned problem (previous patch only fixed the case of grub - grub
>> chainloading, this patch should properly parse image path also when called
>> from other EFI application).
>>
> I'm OK with this patch. Especially that it's unlikely to break anything. Is

Committed.

> the other patch still needed? If other loaders do it with a single path
> element we should probably too, to avoid this kind of bugs. Question is
> mostly whether it's rc1 material.
> 

It is not needed. I would still consider it long term - it makes code
less confusing and avoids probably less-utilized code paths elsewhere.

It also demonstrates that we desperately need EFI boot tests. Much
thanks to SUSE for setting up comprehensive test suite!


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: pvgrub2 legacy_configfile issues: /sbin/init exec error -8

[Andrei Borzenkov]

25.02.2017 02:22, Vladimir 'phcoder' Serbinenko пишет:
> On Sun, Feb 12, 2017, 11:52 Vladimir 'phcoder' Serbinenko 
> wrote:
> 
>>
>>
>> On Sun, 12 Feb 2017, 08:06 Andrei Borzenkov  wrote:
>>
>> 12.02.2017 00:07, Vladimir 'phcoder' Serbinenko пишет:
>> ...

>>> For multiboot module old grub was passing module file name on module
>>> command line, so legacy_configfile should replicate this behaviour. But
>> in
>>> Linux case it shouldn't have ended up loading twice. We need additional
>>> code in legacy_initrd. Please try the attached patch
>>>
>> ...
>>>
>>>
>>> diff --git a/grub-core/commands/legacycfg.c
>> b/grub-core/commands/legacycfg.c
>>> index dd9d9f1..24546bf 100644
>>> --- a/grub-core/commands/legacycfg.c
>>> +++ b/grub-core/commands/legacycfg.c
>>> @@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
>> __attribute__ ((unused)),
>>>  #endif
>>>  );
>>>
>>> -  return cmd->func (cmd, argc, args);
>>> +  return cmd->func (cmd, argc ? argc - 1 : 0, args + 1);
>>
>> If the goal is to be compatible with legacy grub, it is not compatible
>> (nor was) - legacy grub "initrd" ignored all extra arguments to "initrd"
>> command while we attempt to load them as additional initrd components.
>>
>> Nor did legacy grub attempt to quote command line for multiboot kernel
>> or modules.
>>
>> Agreed on both. That's why I said that previous patch is wrong and I'm
>> working on new one
>>
> For first point, see attached patch.
> Second point isn't a problem as legacycfg will already split along spaces
> and hence quoting code will not be triggered.
> 

It will.

  while (*c)
{
  if (*c == '\\' || *c == '\'' || *c == '"')
*buf++ = '\\';
  *buf++ = *c;
  c++;
}

In particular, this is a problem with Linux which does not interpret
these quotes at all (it only supports "  " without any way to escape
nested `"'). That's something for post 2.02 now, but I still would like
to know what was the reason for this code in the first place.

> 
> diff --git a/grub-core/commands/legacycfg.c b/grub-core/commands/legacycfg.c
> index dd9d9f1..b32f3c7 100644
> --- a/grub-core/commands/legacycfg.c
> +++ b/grub-core/commands/legacycfg.c
> @@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
> __attribute__ ((unused)),
> #endif
> );
> 
> - return cmd->func (cmd, argc, args);
> + return cmd->func (cmd, argc ? 1 : 0, args);
> }
> if (kernel_type == MULTIBOOT)
> {
> 

Looks OK as stop gap. @Andy, could you test it? 

I still do not understand why initrd is parsed as TYPE_FILE_NO_CONSUME,
nor why we handle multiboot kernel in legacy_initrd in the first place.
Legacy grub only supported Linux kernel with initrd command.

  switch (kernel_type)
{
case KERNEL_TYPE_LINUX:
case KERNEL_TYPE_BIG_LINUX:
  if (! load_initrd (arg))
return 1;
  break;

default:
  errnum = ERR_NEED_LX_KERNEL;
  return 1;
}

Although this probably does not really matter now.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 1/1 V3] add --partuuid to probe

[Steve Kenton]

On 02/21/2017 05:44 PM, Steve Kenton wrote:


Support both EFI and NT Disk Signature for passing to kernel as 
root=PARTUUID=$val

Signed-off-by: Steve Kenton 
---
V3 revert bogus index change it V2, more style cleanups, skip nested partitions

This boots Ubuntu 16.04 properly, verified my checking /proc/cmdline

However, on an embedded system using Buildroot the system boots but shows this
message and sysfs does not get mounted, udev does not run etc. I thinks it's a
problem with Buildroot/Busybox but wanted to mention it. The system is usable
once sysfs is manually mounted on /sys.

mount: libmount/src/tab_parse.c:706: mnt_table_parse_stream: Assertion 
`fs->refcount == 1' failed.



Apparently a bug in util-linux-2.28 libmount, already fixed in
upstream/stable 2.28.2

FYI - Steve




___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: pvgrub2 legacy_configfile issues: /sbin/init exec error -8

[Vladimir 'phcoder' Serbinenko]

On Sun, Feb 12, 2017, 11:52 Vladimir 'phcoder' Serbinenko 
wrote:

>
>
> On Sun, 12 Feb 2017, 08:06 Andrei Borzenkov  wrote:
>
> 12.02.2017 00:07, Vladimir 'phcoder' Serbinenko пишет:
> ...
> >>
> > For multiboot module old grub was passing module file name on module
> > command line, so legacy_configfile should replicate this behaviour. But
> in
> > Linux case it shouldn't have ended up loading twice. We need additional
> > code in legacy_initrd. Please try the attached patch
> >
> ...
> >
> >
> > diff --git a/grub-core/commands/legacycfg.c
> b/grub-core/commands/legacycfg.c
> > index dd9d9f1..24546bf 100644
> > --- a/grub-core/commands/legacycfg.c
> > +++ b/grub-core/commands/legacycfg.c
> > @@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
> __attribute__ ((unused)),
> >  #endif
> >  );
> >
> > -  return cmd->func (cmd, argc, args);
> > +  return cmd->func (cmd, argc ? argc - 1 : 0, args + 1);
>
> If the goal is to be compatible with legacy grub, it is not compatible
> (nor was) - legacy grub "initrd" ignored all extra arguments to "initrd"
> command while we attempt to load them as additional initrd components.
>
> Nor did legacy grub attempt to quote command line for multiboot kernel
> or modules.
>
> Agreed on both. That's why I said that previous patch is wrong and I'm
> working on new one
>
For first point, see attached patch.
Second point isn't a problem as legacycfg will already split along spaces
and hence quoting code will not be triggered.


diff --git a/grub-core/commands/legacycfg.c b/grub-core/commands/legacycfg.c
index dd9d9f1..b32f3c7 100644
--- a/grub-core/commands/legacycfg.c
+++ b/grub-core/commands/legacycfg.c
@@ -517,7 +517,7 @@ grub_cmd_legacy_initrd (struct grub_command *mycmd
__attribute__ ((unused)),
#endif
);

- return cmd->func (cmd, argc, args);
+ return cmd->func (cmd, argc ? 1 : 0, args);
}
if (kernel_type == MULTIBOOT)
{
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: strip off final NULL from File Path in grub_efi_get_filename

[Vladimir 'phcoder' Serbinenko]

On Fri, Feb 24, 2017, 09:47 Andrei Borzenkov  wrote:

> UEFI 2.6 9.3.6.4 File Path Media Device Path says that Path Name is
> "A NULL-terminated Path string including directory and file names".
>
> Strip final NULL from Path Name in each File Path node when constructing
> full path. To be on safe side, strip all of them.
>
> Fixes failure chainloading grub from grub, when loaded grub truncates
> image path and does not find its grub.cfg.
>
> https://bugzilla.opensuse.org/show_bug.cgi?id=1026344
>
> This was triggered by commit ce95549cc54b5d6f494608a7c390dba3aab4fba7;
> before it we built Path Name without trailing NULL, and apparently all
> other bootloaders use single File Path node, thus not exposing this bug.
>
> ---
> @Vladimir, @Daniel - this fixes regression in rc1 and is real fix for
> the mentioned problem (previous patch only fixed the case of grub - grub
> chainloading, this patch should properly parse image path also when called
> from other EFI application).
>
I'm OK with this patch. Especially that it's unlikely to break anything. Is
the other patch still needed? If other loaders do it with a single path
element we should probably too, to avoid this kind of bugs. Question is
mostly whether it's rc1 material.

>
>  grub-core/kern/efi/efi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index caf9bcc..d467785 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -366,6 +366,9 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
>   len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4)
>  / sizeof (grub_efi_char16_t));
>   fp = (grub_efi_file_path_device_path_t *) dp;
> + /* According to EFI spec Path Name is NULL terminated */
> + while (len > 0 && fp->path_name[len - 1] == 0)
> +   len--;
>
>   p = (char *) grub_utf16_to_utf8 ((unsigned char *) p,
> fp->path_name, len);
> }
> --
> tg: (2fb8cd2..) u/efi-strip-final-NULL-from-file-path (depends on: master)
>
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

Hi folks,

It would be nice to see this clear bug fixed for the upcoming release
- any chance of getting it reviewed please?

-- 
Steve McIntyre, Cambridge, UK.st...@einval.com
You raise the blade, you make the change... You re-arrange me 'til I'm sane...


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3] disk/mdraid1x: Fix >2TB RAID detection with BIOS

[Robert LeBlanc]

On Wed, Jan 25, 2017 at 3:02 PM, Robert LeBlanc  wrote:
> Changes in v3:
>   - Fix to return if not out of range instead of breaking out of the
> loop.
>
> Changes in v2:
>   - Only continue checking for other metadata versions if we get an out
> of range error and reset grub_errno if we continue.
>
> When a mdadm RAID array is on a drive larger than 2TB, the array is not
> able to be detected and as such even if the array has a partition that
> holds /boot under the 2TB limit, it is unable to boot the machine. This
> is caused by metadata 1.0 being tested first which allocates the
> superblock at the end of the device. When it tries to access the end of
> the device it throws an error and the code returns without trying to
> find the superblock at other locations (metadata 1.1 and 1.2). This
> patch changes the error to not be fatal and allow for the checking for
> the other metadata versions and allowing the machine to boot as long as
> /boot is under the 2TB BIOS limit. This won't cause issues with 1.0
> metadata because GRUB is able to read the partitions from the front of
> the drive/partition without having to determine the data offset, since
> the data for metadata 1.0 starts at sector 0.
>
> Signed-off-by: Robert LeBlanc 
> ---
>  grub-core/disk/mdraid1x_linux.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/mdraid1x_linux.c b/grub-core/disk/mdraid1x_linux.c
> index 7cc80d3df..f0aeb6829 100644
> --- a/grub-core/disk/mdraid1x_linux.c
> +++ b/grub-core/disk/mdraid1x_linux.c
> @@ -148,7 +148,17 @@ grub_mdraid_detect (grub_disk_t disk,
>
>if (grub_disk_read (disk, sector, 0, sizeof (struct 
> grub_raid_super_1x),
>   ))
> -   return NULL;
> +   {
> + if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> +   {
> + grub_errno = GRUB_ERR_NONE;
> + continue;
> +   }
> +  else
> +   {
> + return NULL;
> +   }
> +   }
>
>if (sb.magic != grub_cpu_to_le32_compile_time (SB_MAGIC)
>   || grub_le_to_cpu64 (sb.super_offset) != sector)


Does this look okay? I'm not seeing this in patchworks to know what
the status is.

Thanks,
Robert LeBlanc

Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: Loading DSDT table using 'acpi' or some memory write command?

[Andrei Borzenkov]

23.02.2017 19:00, Nando Eva пишет:
> Hi grub-devel, I'm endeavouring to pre-load a modified DSDT table 
> using grub2. The syntax I've tried being as shown below, after which 
> I chainload to Win10. This being done on a Dell E6540 with Win10 and 
> grub 2.02 (or older.. tried many versions, same result).
> 
> acpi /efi/dsdt.amlacpi --load-table dsdt /efi/dsdt.amlchainloader 
> /efi/Boot/bootx64.efi Then I do 'acpidump -b' to peruse the DSDT 
> table. In all cases, it remains the same system BIOS one, not my 
> modified /efi/dsdt.aml one.
> 
> I've also set 'set debug=all' and can see grub2 is reading my 
> modified dsdt file and doing memory writes. So two questions: 1. Is 
> 'acpi' not capable of loading a DSDT table for use with Win10?

It is supposed to do it. Could you test with explicit "acpi -(1|2)" to
force either version 1 or version 2 of tables.

> 2. if that is the case, is there some 'memload' or 'writefromfile' 
> type command where I give a memory address and a file which grub2
> can then write? The idea is simply to replace the existing DSDT at
> it's address with one given by the file of same size or smaller.
> Thank you,Nando
> 


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] squash4: fix handling of fragments and sparse files

[Andrei Borzenkov]

23.02.2017 16:52, Vladimir 'phcoder' Serbinenko пишет:
> On Sat, Feb 18, 2017, 10:17 Andrei Borzenkov  wrote:
> 
>> 1. Do not assume block list and fragment are mutually exclusive. Squash
>> can pack file tail as fragment (unless -no-fragments is specified); so
>> check read offset and read either from block list or from fragments as
>> appropriate.
>>
>> 2. Support sparse files with zero blocks.
>>
>> 3. Fix fragment read - frag.offset is absolute fragment position,
>> not offset relative to ino.chunk.
>>
> Go ahead.
> 

Done with cosmetic changes (renamed one variable to better match
surrounding code and removed now superfluous assignment).

>>
>> Reported and tested by Carlo Caione 
>>
>> ---
>>
>> @Vladimir: we need regression tests for both of these cases. We could real
>> small
>> files only by accident - block list was zero, so it appeared to work.
>>
> How do you create those files reliably? Feel free to add any files to
> grub-fs-tester. Feel free to commit without tests, we can add tests later.
> 

I would say, we need to generally create less "nice" test files

1. Generate test files that are not exact multiple of filesystem block.
This would cover squash tail packing and may uncover similar issues in
other drivers as well.

2. Generate sparse files by seeking beyond end of file. We already had
similar problem with indirect ext* blocks that was not caught by tests.
We should produce holes both for direct and indirect blocks (we may pick
common offset or make it fs-specific if necessary).

3. We need to produce really small files also to test inlining. Again we
can pick common size or make it fs-specific.

Assuming we generate files as described, for squash4 just call it with
-always-use-fragments to force tail packing. -nopad may be interesting
as well to stress our driver even more.



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH] efi: strip off final NULL from File Path in grub_efi_get_filename

[Andrei Borzenkov]

UEFI 2.6 9.3.6.4 File Path Media Device Path says that Path Name is
"A NULL-terminated Path string including directory and file names".

Strip final NULL from Path Name in each File Path node when constructing
full path. To be on safe side, strip all of them.

Fixes failure chainloading grub from grub, when loaded grub truncates
image path and does not find its grub.cfg.

https://bugzilla.opensuse.org/show_bug.cgi?id=1026344

This was triggered by commit ce95549cc54b5d6f494608a7c390dba3aab4fba7;
before it we built Path Name without trailing NULL, and apparently all
other bootloaders use single File Path node, thus not exposing this bug.

---
@Vladimir, @Daniel - this fixes regression in rc1 and is real fix for
the mentioned problem (previous patch only fixed the case of grub - grub
chainloading, this patch should properly parse image path also when called
from other EFI application).

 grub-core/kern/efi/efi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index caf9bcc..d467785 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -366,6 +366,9 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
  len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4)
 / sizeof (grub_efi_char16_t));
  fp = (grub_efi_file_path_device_path_t *) dp;
+ /* According to EFI spec Path Name is NULL terminated */
+ while (len > 0 && fp->path_name[len - 1] == 0)
+   len--;
 
  p = (char *) grub_utf16_to_utf8 ((unsigned char *) p, fp->path_name, 
len);
}
-- 
tg: (2fb8cd2..) u/efi-strip-final-NULL-from-file-path (depends on: master)

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel