Re: [PATCH] at_keyboard: Fix keyboards that report IBM PC AT scan codes
Hi Javier and Renaud,
Sorry for late reply but I am busy with other important stuff.
I will try to take a look at the patch next week.
Daniel
On Tue, Jan 19, 2021 at 09:07:30AM +0100, Javier Martinez Canillas wrote:
> Hello Vladimir and Daniel,
>
> Could you please let us know your thoughts on Renaud's investigation
> and patches? Thanks a lot for your help.
>
> On 1/8/21 3:00 PM, Renaud Métrich wrote:
> > Hello,
> >
> > It appears that the proposal works fine on all systems I could test
> > except one: a HP DL380p Gen 8.
> >
> > On that system, querying the set fails: the ACK bytes in write_mode(0)
> > work perfectly, but then 0xFE ("NACK") is read, as if the keyboard
> > didn't want to send back the set in use.
> >
> > Hence, query_mode() returns 0, causing set1 to be used, but
> > unfortunately the system expects set2 to be used.
> >
> > I tried using the "resend" command, but nothing helps.
> >
> > I'm hence proposing a fallback solution for this kind of hardware where
> > the admin can add a "at_keyboard_fallback_set" environment variable in
> > grub.cfg that would end using a specific set if query_mode() fails.
> >
> > See proposed patch in attachment.
> >
> > Renaud.
> >
> > On 12/14/20 5:47 PM, Renaud Métrich wrote:
> >> Hi Vladimir,
> >>
> >> Thanks for the hint, this was obvious now.
> >>
> >> Please find attached the new patch which definitely fixes the issue.
> >>
> >> It has been tested on various hardware (see git commit details).
> >>
> >> In a nutshell the solution is to stick to set 1 if controller is in
> >> Translate mode, and use set X otherwise, X being the queried mode.
> >>
> >> Additionally, in controller_fini, nothing has to be restored, since
> >> nothing was changed. This fixes an issue when switching between
> >> at_keyboard, console, and at_keyboard again, in case queried set is
> >> not the actual used set.
> >>
> >> Renaud.
> >>
>
> Best regards,
> --
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
IS: GRUB release cycle: WAS: Re: [PATCH v3 3/5] argon2: Import Argon2 from cryptsetup
On Tue, Jan 19, 2021 at 08:31:03PM +0100, Petr Vorel wrote: > Hi Patrick, > > > On Tue, Jan 19, 2021 at 02:06:15PM +0100, Petr Vorel wrote: > > > Hi, > > > > > Please try to build only for the "efi-64" platform. (not "pc") > > > right, I'll test it, but we need to support also legacy bios. > > > I wasn't aware it wouldn't build on the "pc" platform, but I never even > > tried in the first place. The patch series definitely needs more > > polishing anyway, but thanks for making me aware. > Thanks! > One more note: "Argon2 not supported" is not shown, instead there is > "Invalid encryption" printed later in luks2_recover_key(). > It was trivial to modify sources to find it, but that will not be user > friendly > for users which will run grub2 from distro package. > > > > BTW as Argon2 is the default PBKDF algorithm in current cryptsetup, Debian > > > installer (and probably other distros' installers) uses it and it has to > > > be > > > converted after installation with Live CD / connecting HDD on different > > > machine. > > > > Therefore postponing Argon2 to another release means users will have it > > > in 2+ > > > years after 2.06 release (unless distros backport it). > > > That cannot be helped, unfortunately. The current release has been > > taking far too long already, and delaying it even further will > > completely bar anybody from using LUKS2 with GRUB in the first place. > Sure, understand. > > > I don't really know about the release cycle, but personally I wouldn't > > mind to accelerate it. That's only my two cents though as somebody who > > hasn't been involved in the project that deeply. > +1 (that would allow to greatly reduce number of patches in distros, but > understand the lack of maintainers time). I hope we will be able to release 2.06 in 2-3 months. Now I am looking at cutting rc1 shortly. Expect it in the following weeks. Definitely I do not want to make current release cycle a norm. I am going to work on improving it after 2.06 release. I think we will be targeting something between 6-12 months. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key
On Tue, 2021-01-05 at 14:03 -0600, Glenn Washburn wrote: > On Mon, 04 Jan 2021 22:48:49 -0800 > James Bottomley wrote: > > > On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote: > > > James, > > > > > > I like the improvements here. However, I've been thinking more > > > about this and other improvements that deal with passing > > > parameters to modules used by cryptomount. I have some ideas that > > > I've not had the time to fully investigate or code up proof of > > > concepts. One idea is that we shouldn't be changing the function > > > declaration of recover key, that is to say adding new parameters. > > > Instead we should be adding the parameters to grub_cryptodisk_t > > > and setting them in grub_cryptodisk_scan_device_real. This would > > > satisfy needs of this patch series as well as the key file, > > > detached header, sending password as cryptomount arg, and master > > > key features without cluttering the function signature. > > > > Keeping large amounts of shared state between caller and callee can > > be a debugger's nightmare. In this case the only consumer of the > > password callback is the recover function, so it seems appropriate > > it should be an argument to that function. > > Please see my recently submitted RFC patch for a more concrete idea > of exactly what I'm suggesting. I don't see the concern about shared > state as being applicable in this case, even though your point is > valid in other situations. Well, I've seen the patch, but it doesn't seem to address the root of the problem of where the password data might be used: if the password data is used by more than one function then it should likely be part of the shared data; if it's only used by a single function it makes more sense as an argument. I think you need to flesh your RFC out further to make that determination. On what is passed, we do have a question about whether it's data or a functional callback. I do tend to prefer callbacks in situations like this because they solve the lifetime issues (secrets should have well defined lifetimes to make sure there's a limited window for leaking them). A simple data pointer doesn't necessarily do this. So I think the important question is functional callback or data and where it's passed is simply a more minor detail the solution to which will become apparent in the use cases and which it's not hugely important to get right in the first instance. James > > > So, I don't think this is the right approach. > > > > The thing this patch demonstrates is that altering the function > > signatures is fairly easy, so it would be a simple patch to alter > > them again if the password callback were decided to be an essential > > component of the cryptodisk device ... but it should really driven > > the need which isn't apparent yet. > > By easy, I take you to mean there aren't a lot of places needing to > be modified because of the change in signature, and in that sense its > easy, which is good. But its also unnecessary. I'm not sure if you've > looked at the struct grub_cryptodisk yet, but its already pretty > cluttered. So I can see why you might not want to add more. However, > I think my solution (again see RFC patch) is the cleaner, more > scalable solution. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk
On Sun, 2021-01-03 at 12:46 +0200, Dov Murik wrote:
> Hello James,
>
> On 31/12/2020 19:36, James Bottomley wrote:
[...]
> > diff --git a/grub-core/disk/efi/efisecret.c b/grub-
> > core/disk/efi/efisecret.c
> > new file mode 100644
> > index 0..318af0784
> > --- /dev/null
> > +++ b/grub-core/disk/efi/efisecret.c
> > @@ -0,0 +1,129 @@
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +static grub_efi_packed_guid_t secret_guid =
> > GRUB_EFI_SECRET_TABLE_GUID;
> > +static grub_efi_packed_guid_t tableheader_guid =
> > GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> > +static grub_efi_packed_guid_t diskpasswd_guid =
> > GRUB_EFI_DISKPASSWD_GUID;
> > +
> > +/*
> > + * EFI places the secret in the lower 4GB, so it uses a UINT32
> > + * for the pointer which we have to transform to the correct type
> > + */
>
> This comment is no longer accurate now that efi_secret has 64-bit
> address and length fields. Just remove it?
Yes, an oversight, I'll update the comment.
> > +struct efi_secret {
> > + grub_uint64_t base;
> > + grub_uint64_t size;
> > +};
> > +
> > +struct secret_header {
> > + grub_efi_packed_guid_t guid;
> > + grub_uint32_t len;
> > +};
> > +
> > +struct secret_entry {
> > + grub_efi_packed_guid_t guid;
> > + grub_uint32_t len;
> > + char data[0];
> > +};
> > +
> > +static grub_err_t
> > +grub_efi_secret_put (const char *arg __attribute__((unused)), int
> > have_it,
> > +char **ptr)
> > +{
> > + struct secret_entry *e = (struct secret_entry *)(*ptr -
> > (long)&((struct secret_entry *)0)->data);
> > +
> > + /* destroy the secret */
> > + grub_memset (e, 0, e->len);
>
> In grub-core/lib/libgcrypt/src/g10lib.h there's a wipememory macro
> which should circumvent optimizers which might remove the grub_memset
> call:
>
>
> /* To avoid that a compiler optimizes certain memset calls away,
> these
> macros may be used instead. */
> #define wipememory2(_ptr,_set,_len) do { \
>volatile char *_vptr=(volatile char *)(_ptr); \
>size_t _vlen=(_len); \
>while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
>} while(0)
> #define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)
>
>
>
> Maybe the same thing should be used here or copied here? This is an
> attempt to wipe the disk decryption password (= part the EFI secret
> area) against future leakage.
>
> (Note that I have no evidence that any optimizer currently removes
> the grub_memset call.)
Actually, this gets into the whole systems annoyance about who should
see the secret and how long should it live. The truth is this secret
has to be used twice: once for grub to find the kernel and initrd and
once for the initrd to mount root. The fact that there's no current
mechanism in place for grub to pass the secret to the initrd is a bit
of a systems failure. I was thinking the config table could do that,
in which case this code should really be eliminated. Likely the best
thing to do would be to have an OVMF exitbootservices notifier which
wipes everything.
> > + *ptr = NULL;
> > +
> > + if (have_it)
> > +return GRUB_ERR_NONE;
> > +
> > + return grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed
> > to unlock any volumes");
> > +}
> > +
> > +static grub_err_t
> > +grub_efi_secret_find (struct efi_secret *s, char **secret_ptr)
> > +{
> > + int len;
> > + struct secret_header *h;
> > + struct secret_entry *e;
> > + unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
>
> The cast to unsigned long can be removed (s->base is a
> grub_uint64_t).
The style of casting is best practice for converting integers to
pointers ... it's actually what the C manual recommends so it's best to
keep it for the compiler to be aware this is exactly what is intended.
Just as an illustration think what happens on a 32 bit machine:
grub_uint64_t is too big and we need the unsigned long cast to truncate
it.
> > +
> > + /* the area must be big enough for a guid and a u32 length */
> > + if (s->size < sizeof (*h))
> > +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small");
> > +
> > + h = (struct secret_header *)ptr;
> > + if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> > +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area
> > does not start with correct guid\n");
> > + if (h->len < sizeof (*h))
> > +return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small\n");
> > +
> > + len = h->len - sizeof (*h);
> > + ptr += sizeof (*h);
> > +
> > + while (len >= (int)sizeof (*e)) {
> > +e = (struct secret_entry *)ptr;
> > +if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area
> > is corrupt\n");
> > +
> > +if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e-
> > >guid))) {
> > + int end = e->len - sizeo
Re: IS: GRUB release cycle: WAS: Re: [PATCH v3 3/5] argon2: Import Argon2 from cryptsetup
Hi Daniel, [Cc Vladimir] ... > > > I don't really know about the release cycle, but personally I wouldn't > > > mind to accelerate it. That's only my two cents though as somebody who > > > hasn't been involved in the project that deeply. > > +1 (that would allow to greatly reduce number of patches in distros, but > > understand the lack of maintainers time). > I hope we will be able to release 2.06 in 2-3 months. Now I am looking > at cutting rc1 shortly. Expect it in the following weeks. That'd be great. > Definitely I do not want to make current release cycle a norm. I am > going to work on improving it after 2.06 release. I think we will be > targeting something between 6-12 months. Anything than current situation will be great. One would say that releasing more often with smaller diff would be better even for you as a maintainer, but maybe the amount work to test everything before the release is huge (given that testing e.g. Adrian does are probably done manually). Busy projects with enough maintainers and other community members which helps with reviewing and with working CI and good test coverage release every 3-4 months. It's clear that Grub isn't in that state, I wonder what could the community do to help you to improve the situation a bit. Kind regards, Petr > Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC] efi spi flash support - where to place
Dear Michael, Am 19.01.21 um 11:38 schrieb Michael Lawnick: first of all, I am new to this group/grub. So please be patient if I violate some group internal rules, just point it out. Formerly I worked many years on U-Boot. Welcome to GRUB, and no worries. Plain text emails, and polite. What more can we wish for. ;-) I have made a driver and command line tool for accessing SPI NOR flash via EFI interface which I would like to upstream. Very nice. Q: where to place the efi driver, where to place the command line handler. Currently I'd say - create grub-core/flash/efispinor.c for the driver I guess, that would be fine, but I found, `grub-core/bus/spi/rk3288_spi.c`, so I suggest to place it there. - create grub-core/commands/efi/efispinor.c for the command line tool. Can you please elaborate on the usage? Couldn’t it be some generic tool, and it doesn’t matter, what interface in the background? […] I am looking forward to this patch. I have no idea, if it will get into the next release. But as it’s a new driver and command not affecting existing stuff, an exception could be made. Kind regards, Paul ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
