| Message ID | 26f7bd89f212f68b03a4b207e96d8702c9049015.1578910723.git.n.merinov@inango-systems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] partitions/efi: Fix partition name parsing in GUID partition entry | expand |
Hello, Did you have a time to look at this patch? Should I make any modification? Regards, Nikolai ----- Original Message ----- > From: "n merinov" <n.merinov@inango-systems.com> > To: hch@infradead.org, "Davidlohr Bueso" <dave@stgolabs.net>, "Jens Axboe" <axboe@kernel.dk>, "Ard Biesheuvel" > <ardb@kernel.org>, linux-efi@vger.kernel.org, linux-block@vger.kernel.org, "linux-kernel" > <linux-kernel@vger.kernel.org> > Cc: "n merinov" <n.merinov@inango-systems.com> > Sent: Monday, January 13, 2020 3:27:23 PM > Subject: [PATCH v3] partitions/efi: Fix partition name parsing in GUID partition entry > GUID partition entry defined to have a partition name as 36 UTF-16LE > code units. This means that on big-endian platforms ASCII symbols > would be read with 0xXX00 efi_char16_t character code. In order to > correctly extract ASCII characters from a partition name field we > should be converted from 16LE to CPU architecture. > > The problem exists on all big endian platforms. > > Signed-off-by: Nikolai Merinov <n.merinov@inango-systems.com> > --- > block/partitions/efi.c | 3 ++- > block/partitions/efi.h | 2 +- > include/linux/efi.h | 5 +++++ > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/block/partitions/efi.c b/block/partitions/efi.c > index db2fef7dfc47..f1d0820de844 100644 > --- a/block/partitions/efi.c > +++ b/block/partitions/efi.c > @@ -715,7 +715,8 @@ int efi_partition(struct parsed_partitions *state) > ARRAY_SIZE(ptes[i].partition_name)); > info->volname[label_max] = 0; > while (label_count < label_max) { > - u8 c = ptes[i].partition_name[label_count] & 0xff; > + u8 c = 0xff & efi_char16le_to_cpu( > + ptes[i].partition_name[label_count]); > if (c && !isprint(c)) > c = '!'; > info->volname[label_count] = c; > diff --git a/block/partitions/efi.h b/block/partitions/efi.h > index 3e8576157575..4d4cae0bb79e 100644 > --- a/block/partitions/efi.h > +++ b/block/partitions/efi.h > @@ -88,7 +88,7 @@ typedef struct _gpt_entry { > __le64 starting_lba; > __le64 ending_lba; > gpt_entry_attributes attributes; > - efi_char16_t partition_name[72 / sizeof (efi_char16_t)]; > + efi_char16le_t partition_name[72 / sizeof(efi_char16le_t)]; > } __packed gpt_entry; > > typedef struct _gpt_mbr_record { > diff --git a/include/linux/efi.h b/include/linux/efi.h > index aa54586db7a5..47882f2d45db 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -45,9 +45,14 @@ > typedef unsigned long efi_status_t; > typedef u8 efi_bool_t; > typedef u16 efi_char16_t; /* UNICODE character */ > +typedef __le16 efi_char16le_t; /* UTF16-LE */ > +typedef __be16 efi_char16be_t; /* UTF16-BE */ > typedef u64 efi_physical_addr_t; > typedef void *efi_handle_t; > > +#define efi_char16le_to_cpu le16_to_cpu > +#define efi_char16be_to_cpu be16_to_cpu > + > /* > * The UEFI spec and EDK2 reference implementation both define EFI_GUID as > * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment > -- > 2.17.1
On Mon, Jan 13, 2020 at 03:27:23PM +0500, Nikolai Merinov wrote: > GUID partition entry defined to have a partition name as 36 UTF-16LE > code units. This means that on big-endian platforms ASCII symbols > would be read with 0xXX00 efi_char16_t character code. In order to > correctly extract ASCII characters from a partition name field we > should be converted from 16LE to CPU architecture. > > The problem exists on all big endian platforms. > > Signed-off-by: Nikolai Merinov <n.merinov@inango-systems.com> > --- > block/partitions/efi.c | 3 ++- > block/partitions/efi.h | 2 +- > include/linux/efi.h | 5 +++++ > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/block/partitions/efi.c b/block/partitions/efi.c > index db2fef7dfc47..f1d0820de844 100644 > --- a/block/partitions/efi.c > +++ b/block/partitions/efi.c > @@ -715,7 +715,8 @@ int efi_partition(struct parsed_partitions *state) > ARRAY_SIZE(ptes[i].partition_name)); > info->volname[label_max] = 0; > while (label_count < label_max) { > - u8 c = ptes[i].partition_name[label_count] & 0xff; > + u8 c = 0xff & efi_char16le_to_cpu( > + ptes[i].partition_name[label_count]); Why are you swapping the order of the comparism to an unusual one here? > - efi_char16_t partition_name[72 / sizeof (efi_char16_t)]; > + efi_char16le_t partition_name[72 / sizeof(efi_char16le_t)]; > } __packed gpt_entry; > > typedef struct _gpt_mbr_record { > diff --git a/include/linux/efi.h b/include/linux/efi.h > index aa54586db7a5..47882f2d45db 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -45,9 +45,14 @@ > typedef unsigned long efi_status_t; > typedef u8 efi_bool_t; > typedef u16 efi_char16_t; /* UNICODE character */ > +typedef __le16 efi_char16le_t; /* UTF16-LE */ > +typedef __be16 efi_char16be_t; /* UTF16-BE */ > typedef u64 efi_physical_addr_t; > typedef void *efi_handle_t; > > +#define efi_char16le_to_cpu le16_to_cpu > +#define efi_char16be_to_cpu be16_to_cpu I'd rather use plain __le16 and le16_to_cpu here. Also the be variants seems to be entirely unused.
Hi Christoph, > I'd rather use plain __le16 and le16_to_cpu here. Also the be > variants seems to be entirely unused. Looks like I misunderstood your comment from https://patchwork.kernel.org/patch/11309223/: > Please add a an efi_char_from_cpu or similarly named helper > to encapsulate this logic. The "le16_to_cpu(ptes[i].partition_name[label_count])" call is the full implementation of the "efi_char_from_cpu" logic. Do you want to encapsulate "utf16_le_to_7bit_string" logic entirely like in the attached version? Regards, Nikolai
On Mon, Feb 24, 2020 at 01:38:39PM +0200, Nikolai Merinov wrote: > Hi Christoph, > > > I'd rather use plain __le16 and le16_to_cpu here. Also the be > > variants seems to be entirely unused. > > Looks like I misunderstood your comment from https://patchwork.kernel.org/patch/11309223/: > > > Please add a an efi_char_from_cpu or similarly named helper > > to encapsulate this logic. > > The "le16_to_cpu(ptes[i].partition_name[label_count])" call is the > full implementation of the "efi_char_from_cpu" logic. Do you want > to encapsulate "utf16_le_to_7bit_string" logic entirely like in > the attached version? I think I though of just the inner loop, but your new version looks even better, so: Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi Christoph, Should I perform any other steps in order to get this change in the master? Regards, Nikolai ----- Original Message ----- > From: "hch" <hch@infradead.org> > To: "n merinov" <n.merinov@inango-systems.com> > Cc: "hch" <hch@infradead.org>, "Davidlohr Bueso" <dave@stgolabs.net>, "Jens Axboe" <axboe@kernel.dk>, "Ard Biesheuvel" > <ardb@kernel.org>, "linux-efi" <linux-efi@vger.kernel.org>, "linux-block" <linux-block@vger.kernel.org>, "linux-kernel" > <linux-kernel@vger.kernel.org> > Sent: Monday, February 24, 2020 10:08:13 PM > Subject: Re: [PATCH v3] partitions/efi: Fix partition name parsing in GUID partition entry > On Mon, Feb 24, 2020 at 01:38:39PM +0200, Nikolai Merinov wrote: >> Hi Christoph, >> >> > I'd rather use plain __le16 and le16_to_cpu here. Also the be >> > variants seems to be entirely unused. >> >> Looks like I misunderstood your comment from >> https://patchwork.kernel.org/patch/11309223/: >> >> > Please add a an efi_char_from_cpu or similarly named helper >> > to encapsulate this logic. >> >> The "le16_to_cpu(ptes[i].partition_name[label_count])" call is the >> full implementation of the "efi_char_from_cpu" logic. Do you want >> to encapsulate "utf16_le_to_7bit_string" logic entirely like in >> the attached version? > > I think I though of just the inner loop, but your new version looks even > better, so: > > Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, 6 Mar 2020 at 09:43, Nikolai Merinov <n.merinov@inango-systems.com> wrote: > > Hi Christoph, > > Should I perform any other steps in order to get this change in the master? > I can take it via the EFI tree with an ack from Dave. > > ----- Original Message ----- > > From: "hch" <hch@infradead.org> > > To: "n merinov" <n.merinov@inango-systems.com> > > Cc: "hch" <hch@infradead.org>, "Davidlohr Bueso" <dave@stgolabs.net>, "Jens Axboe" <axboe@kernel.dk>, "Ard Biesheuvel" > > <ardb@kernel.org>, "linux-efi" <linux-efi@vger.kernel.org>, "linux-block" <linux-block@vger.kernel.org>, "linux-kernel" > > <linux-kernel@vger.kernel.org> > > Sent: Monday, February 24, 2020 10:08:13 PM > > Subject: Re: [PATCH v3] partitions/efi: Fix partition name parsing in GUID partition entry > > > On Mon, Feb 24, 2020 at 01:38:39PM +0200, Nikolai Merinov wrote: > >> Hi Christoph, > >> > >> > I'd rather use plain __le16 and le16_to_cpu here. Also the be > >> > variants seems to be entirely unused. > >> > >> Looks like I misunderstood your comment from > >> https://patchwork.kernel.org/patch/11309223/: > >> > >> > Please add a an efi_char_from_cpu or similarly named helper > >> > to encapsulate this logic. > >> > >> The "le16_to_cpu(ptes[i].partition_name[label_count])" call is the > >> full implementation of the "efi_char_from_cpu" logic. Do you want > >> to encapsulate "utf16_le_to_7bit_string" logic entirely like in > >> the attached version? > > > > I think I though of just the inner loop, but your new version looks even > > better, so: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, 6 Mar 2020 at 10:25, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 6 Mar 2020 at 09:43, Nikolai Merinov > <n.merinov@inango-systems.com> wrote: > > > > Hi Christoph, > > > > Should I perform any other steps in order to get this change in the master? > > > > I can take it via the EFI tree with an ack from Dave. > ... or actually, I'm sure Dave is fine with it, so I'll just queue it in efi/next directly (with Christoph's ack) > > > > > ----- Original Message ----- > > > From: "hch" <hch@infradead.org> > > > To: "n merinov" <n.merinov@inango-systems.com> > > > Cc: "hch" <hch@infradead.org>, "Davidlohr Bueso" <dave@stgolabs.net>, "Jens Axboe" <axboe@kernel.dk>, "Ard Biesheuvel" > > > <ardb@kernel.org>, "linux-efi" <linux-efi@vger.kernel.org>, "linux-block" <linux-block@vger.kernel.org>, "linux-kernel" > > > <linux-kernel@vger.kernel.org> > > > Sent: Monday, February 24, 2020 10:08:13 PM > > > Subject: Re: [PATCH v3] partitions/efi: Fix partition name parsing in GUID partition entry > > > > > On Mon, Feb 24, 2020 at 01:38:39PM +0200, Nikolai Merinov wrote: > > >> Hi Christoph, > > >> > > >> > I'd rather use plain __le16 and le16_to_cpu here. Also the be > > >> > variants seems to be entirely unused. > > >> > > >> Looks like I misunderstood your comment from > > >> https://patchwork.kernel.org/patch/11309223/: > > >> > > >> > Please add a an efi_char_from_cpu or similarly named helper > > >> > to encapsulate this logic. > > >> > > >> The "le16_to_cpu(ptes[i].partition_name[label_count])" call is the > > >> full implementation of the "efi_char_from_cpu" logic. Do you want > > >> to encapsulate "utf16_le_to_7bit_string" logic entirely like in > > >> the attached version? > > > > > > I think I though of just the inner loop, but your new version looks even > > > better, so: > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/block/partitions/efi.c b/block/partitions/efi.c index db2fef7dfc47..f1d0820de844 100644 --- a/block/partitions/efi.c +++ b/block/partitions/efi.c @@ -715,7 +715,8 @@ int efi_partition(struct parsed_partitions *state) ARRAY_SIZE(ptes[i].partition_name)); info->volname[label_max] = 0; while (label_count < label_max) { - u8 c = ptes[i].partition_name[label_count] & 0xff; + u8 c = 0xff & efi_char16le_to_cpu( + ptes[i].partition_name[label_count]); if (c && !isprint(c)) c = '!'; info->volname[label_count] = c; diff --git a/block/partitions/efi.h b/block/partitions/efi.h index 3e8576157575..4d4cae0bb79e 100644 --- a/block/partitions/efi.h +++ b/block/partitions/efi.h @@ -88,7 +88,7 @@ typedef struct _gpt_entry { __le64 starting_lba; __le64 ending_lba; gpt_entry_attributes attributes; - efi_char16_t partition_name[72 / sizeof (efi_char16_t)]; + efi_char16le_t partition_name[72 / sizeof(efi_char16le_t)]; } __packed gpt_entry; typedef struct _gpt_mbr_record { diff --git a/include/linux/efi.h b/include/linux/efi.h index aa54586db7a5..47882f2d45db 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -45,9 +45,14 @@ typedef unsigned long efi_status_t; typedef u8 efi_bool_t; typedef u16 efi_char16_t; /* UNICODE character */ +typedef __le16 efi_char16le_t; /* UTF16-LE */ +typedef __be16 efi_char16be_t; /* UTF16-BE */ typedef u64 efi_physical_addr_t; typedef void *efi_handle_t; +#define efi_char16le_to_cpu le16_to_cpu +#define efi_char16be_to_cpu be16_to_cpu + /* * The UEFI spec and EDK2 reference implementation both define EFI_GUID as * struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied alignment
GUID partition entry defined to have a partition name as 36 UTF-16LE code units. This means that on big-endian platforms ASCII symbols would be read with 0xXX00 efi_char16_t character code. In order to correctly extract ASCII characters from a partition name field we should be converted from 16LE to CPU architecture. The problem exists on all big endian platforms. Signed-off-by: Nikolai Merinov <n.merinov@inango-systems.com> --- block/partitions/efi.c | 3 ++- block/partitions/efi.h | 2 +- include/linux/efi.h | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-)