Re: BUG: Out of bounds read in hci_le_ext_adv_report_evt()

2021-03-21 Thread Emil Lenngren
Hi,

Den mån 22 mars 2021 kl 00:01 skrev Luiz Augusto von Dentz
:
> Or we do something like
> https://lore.kernel.org/linux-bluetooth/20201024002251.1389267-1-luiz.de...@gmail.com/,
> that said the reason we didn't applied my patches was that the
> controller would be the one generating invalid data, but it seems you
> are reproducing with vhci controller which is only used for emulating
> a controller and requires root privileges so it is unlikely these
> conditions would happens with hardware itself, in the other hand as
> there seems to be more and more reports using vhci to emulate broken
> events it perhaps more productive to introduce proper checks for all
> events so we don't have to deal with more reports like this in the
> future.

Keep in mind that when using the H4 uart protocol without any error
correction (as H5 has), it is possible that random bit errors occur on
the wire. I wouldn't like my kernel to crash due to this. Bit errors
happen all the time on RPi 4 for example at the default baud rate if
you just do some heavy stress testing, or use an application that
transfers a lot of data over Bluetooth.

/Emil


Re: [PATCH] ubifs: Add support for zstd compression.

2019-06-07 Thread Emil Lenngren
Hi,

Den fre 7 juni 2019 kl 22:49 skrev Richard Weinberger :
>
> - Ursprüngliche Mail -
> > Von: "Emil Lenngren" 
> > An: "richard" 
> > CC: "linux-mtd" , "Sebastian Andrzej 
> > Siewior" , "linux-kernel"
> > , "Michele Dionisio" 
> > 
> > Gesendet: Freitag, 7. Juni 2019 22:27:09
> > Betreff: Re: [PATCH] ubifs: Add support for zstd compression.
> >> So I'm not sure what is the best choice for the default filesystem.
> >
> > My idea was at the end, i.e. it will only be used when LZO and ZLIB
> > are not selected to be included for UBIFS, for example when someone
> > compiles a minimal kernel who knows exactly which compression
> > algorithms will be used on that system.
>
> BTW: you can always select the compressor using the compr= mount option.
> Also for the default filesystem.

Yep that's what I'm using while I'm testing.

> Putting it at the end does not harm but IMHO the use is little.
> But for the sake of completes, I agree with you. Can you send a follow-up
> patch?

Ok

>
> > I did a single test today and compared lzo and zstd and on that test
> > lzo had faster decompression speed but resulted in larger space. I'll
> > do more tests later.
>
> Can you please share more details? I'm interested what CPU this was.

ARM Cortex-A7. The kernel is compiled with gcc 7.3.1. Next week I'll
test some more.
I have a question about how the decompression is done while reading.
When a large file is read from the filesystem (assuming not in any
cache), is it the case that first a chunk is read from flash, that
chunk is then decompressed, later next chunk is read from flash, that
one is then decompressed and so on, or can the decompression be done
in parallel while reading the next chunk from flash?

/Emil


Re: [PATCH] ubifs: Add support for zstd compression.

2019-06-07 Thread Emil Lenngren
Hi,

Den fre 7 juni 2019 kl 22:09 skrev Richard Weinberger :
>
> Emil,
>
> - Ursprüngliche Mail -
> > In fs/ubifs/sb.c we have
> >
> > static int get_default_compressor(struct ubifs_info *c)
> > {
> >if (ubifs_compr_present(c, UBIFS_COMPR_LZO))
> >return UBIFS_COMPR_LZO;
> >
> >if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB))
> >return UBIFS_COMPR_ZLIB;
> >
> >return UBIFS_COMPR_NONE;
> > }
> >
> > Maybe add an entry for zstd here as well?
>
> Where would you add it? If we add it after UBIFS_COMPR_ZLIB,
> it will effectively never get selected, unless someone builds a kernel
> without lzo and zlib but zstd.
> If we add it before UBIFS_COMPR_ZLIB, it will be used always and users
> end up with unreadable files if they reboot to an older kernel.
> Please note that we didn't raise the UBIFS format version for zstd.
>
> So I'm not sure what is the best choice for the default filesystem.

My idea was at the end, i.e. it will only be used when LZO and ZLIB
are not selected to be included for UBIFS, for example when someone
compiles a minimal kernel who knows exactly which compression
algorithms will be used on that system. I guess that's the same reason
why zlib exists at all and is placed after lzo.
But of course the other positions also have their pros. If we perform
more benchmarks speed/size and conclude that zstd outperforms zlib for
all aspects, then maybe placing it between lzo and zlib could be an
option, but as you say we should avoid breaking compatibility with
older systems.
I did a single test today and compared lzo and zstd and on that test
lzo had faster decompression speed but resulted in larger space. I'll
do more tests later.

/Emil


Re: [PATCH] ubifs: Add support for zstd compression.

2019-06-07 Thread Emil Lenngren
Hello,

Den ons 15 maj 2019 kl 23:03 skrev Richard Weinberger :
>
> From: Michele Dionisio 
>
> zstd shows a good compression rate and is faster than lzo,
> also on slow ARM cores.
>
> Cc: Sebastian Andrzej Siewior 
> Signed-off-by: Michele Dionisio 
> [rw: rewrote commit message]
> Signed-off-by: Richard Weinberger 
> ---
>  fs/ubifs/Kconfig   | 10 ++
>  fs/ubifs/compress.c| 27 ++-
>  fs/ubifs/super.c   |  2 ++
>  fs/ubifs/ubifs-media.h |  2 ++
>  4 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
> index 9da2f135121b..8d84d2ed096d 100644
> --- a/fs/ubifs/Kconfig
> +++ b/fs/ubifs/Kconfig
> @@ -5,8 +5,10 @@ config UBIFS_FS
> select CRYPTO if UBIFS_FS_ADVANCED_COMPR
> select CRYPTO if UBIFS_FS_LZO
> select CRYPTO if UBIFS_FS_ZLIB
> +   select CRYPTO if UBIFS_FS_ZSTD
> select CRYPTO_LZO if UBIFS_FS_LZO
> select CRYPTO_DEFLATE if UBIFS_FS_ZLIB
> +   select CRYPTO_ZSTD if UBIFS_FS_ZSTD
> select CRYPTO_HASH_INFO
> select UBIFS_FS_XATTR if FS_ENCRYPTION
> depends on MTD_UBI
> @@ -37,6 +39,14 @@ config UBIFS_FS_ZLIB
> help
>   Zlib compresses better than LZO but it is slower. Say 'Y' if unsure.
>
> +config UBIFS_FS_ZSTD
> +   bool "ZSTD compression support" if UBIFS_FS_ADVANCED_COMPR
> +   depends on UBIFS_FS
> +   default y
> +   help
> + ZSTD compresses is a big win in speed over Zlib and
> + in compression ratio over LZO. Say 'Y' if unsure.
> +
>  config UBIFS_ATIME_SUPPORT
> bool "Access time support"
> default n
> diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
> index 565cb56d7225..89183aeeeb7a 100644
> --- a/fs/ubifs/compress.c
> +++ b/fs/ubifs/compress.c
> @@ -71,6 +71,24 @@ static struct ubifs_compressor zlib_compr = {
>  };
>  #endif
>
> +#ifdef CONFIG_UBIFS_FS_ZSTD
> +static DEFINE_MUTEX(zstd_enc_mutex);
> +static DEFINE_MUTEX(zstd_dec_mutex);
> +
> +static struct ubifs_compressor zstd_compr = {
> +   .compr_type = UBIFS_COMPR_ZSTD,
> +   .comp_mutex = &zstd_enc_mutex,
> +   .decomp_mutex = &zstd_dec_mutex,
> +   .name = "zstd",
> +   .capi_name = "zstd",
> +};
> +#else
> +static struct ubifs_compressor zstd_compr = {
> +   .compr_type = UBIFS_COMPR_ZSTD,
> +   .name = "zstd",
> +};
> +#endif
> +
>  /* All UBIFS compressors */
>  struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
>
> @@ -228,13 +246,19 @@ int __init ubifs_compressors_init(void)
> if (err)
> return err;
>
> -   err = compr_init(&zlib_compr);
> +   err = compr_init(&zstd_compr);
> if (err)
> goto out_lzo;
>
> +   err = compr_init(&zlib_compr);
> +   if (err)
> +   goto out_zstd;
> +
> ubifs_compressors[UBIFS_COMPR_NONE] = &none_compr;
> return 0;
>
> +out_zstd:
> +   compr_exit(&zstd_compr);
>  out_lzo:
> compr_exit(&lzo_compr);
> return err;
> @@ -247,4 +271,5 @@ void ubifs_compressors_exit(void)
>  {
> compr_exit(&lzo_compr);
> compr_exit(&zlib_compr);
> +   compr_exit(&zstd_compr);
>  }
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 04b8ecfd3470..ea8615261936 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -1055,6 +1055,8 @@ static int ubifs_parse_options(struct ubifs_info *c, 
> char *options,
> c->mount_opts.compr_type = UBIFS_COMPR_LZO;
> else if (!strcmp(name, "zlib"))
> c->mount_opts.compr_type = UBIFS_COMPR_ZLIB;
> +   else if (!strcmp(name, "zstd"))
> +   c->mount_opts.compr_type = UBIFS_COMPR_ZSTD;
> else {
> ubifs_err(c, "unknown compressor \"%s\"", 
> name); //FIXME: is c ready?
> kfree(name);
> diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h
> index 8b7c1844014f..697b1b89066a 100644
> --- a/fs/ubifs/ubifs-media.h
> +++ b/fs/ubifs/ubifs-media.h
> @@ -348,12 +348,14 @@ enum {
>   * UBIFS_COMPR_NONE: no compression
>   * UBIFS_COMPR_LZO: LZO compression
>   * UBIFS_COMPR_ZLIB: ZLIB compression
> + * UBIFS_COMPR_ZSTD: ZSTD compression
>   * UBIFS_COMPR_TYPES_CNT: count of supported compression types
>   */
>  enum {
> UBIFS_COMPR_NONE,
> UBIFS_COMPR_LZO,
> UBIFS_COMPR_ZLIB,
> +   UBIFS_COMPR_ZSTD,
> UBIFS_COMPR_TYPES_CNT,
>  };
>
> --
> 2.16.4

In fs/ubifs/sb.c we have

static int get_default_compressor(struct ubifs_info *c)
{
if (ubifs_compr_present(c, UBIFS_COMPR_LZO))
return UBIFS_COMPR_LZO;

if (ubifs_compr_present(c, UBIFS_COMPR_ZLIB))
return UBIFS_COMPR_ZLIB;

return UBIFS_COMPR_NONE;
}

Maybe add an entry for zstd here as well?

/Emil


Re: [PATCH] pre-shared passcode: secure pairing for "no keyboard, no display" devices

2019-02-15 Thread Emil Lenngren
Hi,
Den fre 15 feb. 2019 kl 12:46 skrev Pavel Machek :
>
> Hi!
>
> > > Currently, "no keyboard, no display" devices can be paired, but
> > > pairing is not secure against active attacker.
> > >
> > > Can we do better? Not for the first pairing; but for the next ones --
> > > yes, I believe we can.
> > >
> > > BLE device in this case has internal storage, and Linux running
> > > there. From factory, random 6-digit number is stored in the
> > > flash. Legitimate user knows the number, and system is manipulated so
> > > that pairing passkey will be this pre-shared passkey. After pairing,
> > > user is allowed to change it.
> > >
> > > [Or maybe passkey is 00 from the factory; this is still win for
> > > the user, as long as he can change the key to something random in a
> > > secure cave.]
> > >
> > > Fortunately, kernel support for this is rather easy; patch is attached
> > > below.
> > >
> > > Does someone see a security issue with proposal above?
> >
> > Assuming "LE Secure Connections" is used, the protocol is only secure
> > if the passkey is not reused. A 6 digit passkey means 20 bits. The
> > protocol is performed in 20 steps, where one bit is compared and
> > revealed in every step. This means the attacker will know for every
> > attempt the first bit that is incorrect in the attempted passkey. If
> > the passkey is reused, the attacker can just try the same passkey with
> > the incorrect bit flipped. In average it takes 10 attempts to crack
> > the key and maximum 20 attempts. Hence, a static key doesn't really
> > add much security compared to Just Works and might give a false sense
> > of security.
>
> Thanks a lot for quick reply. And no, that is not good.
>
> Just Works basically means that there's no security at all, anyone
> within range can connect when owner is not around, and do whatever
> they want.
>
> Is there some common way this is solved?
>
> We do have pre-shared passkey, if only 20 bits. If we set
> passkey = sha( pre-shared passkey + pairing_attempt++ ), would we get
> some kind of meaningful security? Perhaps with limiting pairing
> attempt to say 10 a day?

I see two issues with that approach. First, the user needs to know the
correct counter of pairing_attempt (how?) and perform the sha
calculation and map that to a passkey. Second, if you eavesdrop a
pairing attempt that succeeds, you can just bruteforce sha over all
possible pre-shared passkeys (1 million if you have 6 digits) times
the number of different pairing_attempt counters you want to test, and
match that with the recorded pairing messages. That shouldn't take
more than a few seconds on a normal PC.

What people really do that wants proper security over BLE when they
need to use static passkeys, is using a PAKE algorithm in the
application layer to establish a session key, and then use some AEAD
cipher with that session key also in the application layer (or
potentially start BLE encryption using that session key as LTK, but I
haven't seen that). Unencrypted BLE is just used then as a transport
layer.

/Emil


Re: [PATCH] pre-shared passcode: secure pairing for "no keyboard, no display" devices

2019-02-14 Thread Emil Lenngren
Hi Pavel,

Den tors 14 feb. 2019 kl 14:59 skrev Pavel Machek :
>
> Hi!
>
> Currently, "no keyboard, no display" devices can be paired, but
> pairing is not secure against active attacker.
>
> Can we do better? Not for the first pairing; but for the next ones --
> yes, I believe we can.
>
> BLE device in this case has internal storage, and Linux running
> there. From factory, random 6-digit number is stored in the
> flash. Legitimate user knows the number, and system is manipulated so
> that pairing passkey will be this pre-shared passkey. After pairing,
> user is allowed to change it.
>
> [Or maybe passkey is 00 from the factory; this is still win for
> the user, as long as he can change the key to something random in a
> secure cave.]
>
> Fortunately, kernel support for this is rather easy; patch is attached
> below.
>
> Does someone see a security issue with proposal above?

Assuming "LE Secure Connections" is used, the protocol is only secure
if the passkey is not reused. A 6 digit passkey means 20 bits. The
protocol is performed in 20 steps, where one bit is compared and
revealed in every step. This means the attacker will know for every
attempt the first bit that is incorrect in the attempted passkey. If
the passkey is reused, the attacker can just try the same passkey with
the incorrect bit flipped. In average it takes 10 attempts to crack
the key and maximum 20 attempts. Hence, a static key doesn't really
add much security compared to Just Works and might give a false sense
of security.

A static key for Legacy Pairing has comparable security as using a
random key. The security is kind of the same as logging in to a
website over unencrypted HTTP, assuming the static key is stored on
the peripheral and the user enters the passkey on the central. In the
case where the passkey is entered by the user on the peripheral
instead of the central, the protocol is completely broken
(https://eprint.iacr.org/2013/309.pdf).

/Emil


Re: [PATCH 2/2] mtd: spinand: micron: Support for all Micron SPI NAND flashes

2019-02-04 Thread Emil Lenngren
Hi,

Den mån 4 feb. 2019 kl 12:18 skrev Shivamurthy Shastri (sshivamurthy)
:
>
> Driver is redesigned using parameter page to support all the Micron
> SPI NAND flashes.
>
> Parameter page of Micron flashes is similar to ONFI parameter table and
> functionality is same, so copied some of the common functions like crc16
> and bit_wise_majority from nand_onfi.c.
>
> This driver is tested using MT29F2G01ABXGD, MT29F4G01ABXFD, MT29F8G01ADXFD,
> MT29F1G01ABXFD.
>

> -static const struct spinand_info micron_spinand_table[] = {
> -   SPINAND_INFO("MT29F2G01ABAGD", 0x24,
> -NAND_MEMORG(1, 2048, 128, 64, 2048, 2, 1, 1),

> +   deviceinfo.memorg.eraseblocks_per_lun =
> +   params->blocks_per_lun * params->lun_count;
> +   deviceinfo.memorg.planes_per_lun = params->lun_count;
> +   deviceinfo.memorg.luns_per_target = 1;
> +   deviceinfo.memorg.ntargets = 1;

> +   __le32 blocks_per_lun;
> +   u8 lun_count;
> +   u8 addr_cycles;
> +   u8 bits_per_cell;
> +   __le16 bb_per_lun;

I have a question about the lun_count. As it is now, the
planes_per_lun parameter is initialized to 2 in NAND_MEMORG. In your
patch, it is instead initialized from the "lun_count" property from
the parameter table. But I looked at a datasheet I found by a simple
Google search (https://www.google.se/search?q=micron+nand+spi+datasheet),
the first hit is to the 1 Gb flash MT29F1G01AAADD. That device clearly
has two planes per lun (you need the "plane select" bit in the
requests), but still, according to the parameter page data structure,
byte 100, Number of logical units is set to 01h. Also, the "blocks per
lun" count, which is called "blocks per unit" is 1024, which should be
512 if this parameter really meant "blocks per plane" and the
calculation in the patch was correct.

As a reference, the 2 Gb version of the Macronix flash
(http://www.macronix.com/Lists/Datasheet/Attachments/6866/MX35LF2GE4AB,%203V,%202Gb,%20v1.5.pdf),
also has two planes per lun. It also sets byte 100, Number of logical
units to 01h.

So what I'm wondering is of course if this parameter is the correct
one to use for planes_per_lun. I tried to locate the correct "planes
per lun" parameter in the table, but didn't find anyone. Maybe it's
the unfortunate fact that "planes per lun" isn't exposed in the
parameter table?

/Emil


Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important

2017-06-11 Thread Emil Lenngren
2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach :
> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook  wrote:
>>
>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo  wrote:
>> > "Jason A. Donenfeld"  writes:
>> >
>> >> Whenever you're comparing two MACs, it's important to do this using
>> >> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>> >> which could then be used to iteratively forge a MAC.
>> >
>> > Do you have any pointers where I could learn more about this?
>>
>> While not using C specifically, this talks about the problem generally:
>> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>>
>
> Sorry for the stupid question, but the MAC address is in plaintext in
> the air anyway or easily accessible via user space tools. I fail to
> see what it is so secret about a MAC address in that code where that
> same MAC address is accessible via myriads of ways.

I think you're mixing up Media Access Control (MAC) addresses with
Message Authentication Code (MAC). The second one is a cryptographic
signature of a message.