Bug#994056: cryptsetup: blkid check fails to take offset option into account
Guilhem Moulin dixit: >first to report it I suppose nobody uses large offset= values. Don't >think adding ‘Depends: bc’ is justified here :-P. Eh, bc’s supposed to be a base tool anyway… >Also in practice I was able to use offset=2⁵⁹ (buster-i386)tglase@tglase:~ $ echo '2^59' | bc 576460752303423488 (buster-i386)tglase@tglase:~ $ echo $(($(echo '2^59' | bc)*512)) 0 (buster-i386)tglase@tglase:~ $ bash bash$ echo $(($(echo '2^59' | bc)*512)) 0 I’d not call this “use”. > with bash, dash, klibc's sh and busybox's sh mksh is also a viable /bin/sh (the /bin/lksh binary), and for that I speak as developer ;) >I'll just ignore the potential overflow. I'll just make the script >choke when the arithmetic operation fails. That’s the problem: the operation does not fail, it “only” overflows. Overflowing *is* permitted (by C UB rules) to do “rm -rf /” even if GCC does not (yet) do that… but even if it wraps around, you get the WRONG values (see above). (buster-i386)tglase@tglase:~ $ lksh -c 'echo $(($(echo "2^40" | bc)*512))' 0 (This is actually what POSIX requires. So bash, dash, etc. are actually noncompliant on 32-bit platforms.) So please, if you’re going to “ignore” this in practice, at least install the code that checks for offset ≤ 4194303. I can provide a corresponding patch, if you want. bye, //mirabilos -- FWIW, I'm quite impressed with mksh interactively. I thought it was much *much* more bare bones. But it turns out it beats the living hell out of ksh93 in that respect. I'd even consider it for my daily use if I hadn't wasted half my life on my zsh setup. :-) -- Frank Terbeck in #!/bin/mksh
Bug#994056: cryptsetup: blkid check fails to take offset option into account
On Fri, 08 Oct 2021 at 15:12:58 +, Thorsten Glaser wrote: >>, so I completed your patch with 2373709bb461a71a7af46e7e9c59355fce63e52e. > > -blkid="$(/sbin/blkid -o value -s TYPE -p ${offset:+-O "$offset"} -- "$dev")" > +blkid="$(/sbin/blkid -o value -s TYPE -p ${offset:+-O "$((offset*512))"} -- > "$dev")" > > That’s only good for offset ≤ 4194303 though and invokes C “Undefined > Behaviour” on larger values. (More on LP64 of course.) Ah right, thanks. Given #-1 is a 12 years old bug and AFAICT you're the first to report it I suppose nobody uses large offset= values. Don't think adding ‘Depends: bc’ is justified here :-P. Also in practice I was able to use offset=2⁵⁹ with bash, dash, klibc's sh and busybox's sh on a stock 32 bytes platform, so I'll just ignore the potential overflow. I'll just make the script choke when the arithmetic operation fails. -- Guilhem. signature.asc Description: PGP signature
Bug#994056: cryptsetup: blkid check fails to take offset option into account
Hi Guilhem, >(And added unit tests for the use case.) thanks! I was more interested in getting my system working and did the fix on the installed system without looking at the source package at first. >Thanks for the patch! FWIW crypttab(5)'s ‘offset=’ passes the value to >`cryptsetup -o` which is a 512 byte sectors count while `blkid -O` wants >bytes *OUCH!* Good catch. >, so I completed your patch with 2373709bb461a71a7af46e7e9c59355fce63e52e. -blkid="$(/sbin/blkid -o value -s TYPE -p ${offset:+-O "$offset"} -- "$dev")" +blkid="$(/sbin/blkid -o value -s TYPE -p ${offset:+-O "$((offset*512))"} -- "$dev")" That’s only good for offset ≤ 4194303 though and invokes C “Undefined Behaviour” on larger values. (More on LP64 of course.) Worse, blkid(8) supports multiplicators, but only KiB and more, not (512-byte) sectors, so to fix this *properly* we’d have to rely on bc(1) or dc(1), which unfortunately Debian does not install by default (unlike Unix). I’d say go for it but I’m not sure how majority-capable my opinion on this is ☻ Other options: • ignore this, which will cause misdetection for large offsets (bah) • document that offsets larger than 4194303 are not supported (not worth much if the underlying cryptsetup does support it) ‣ option: check for that • check for large offsets and either “defuse” (skip and permit) or “refuse” (skip and deny) the blkid and un_blkid checks Checking for large offsets is also not trivial, as you cannot do just $((offset < 4194303) for the same reason. In POSIX sh this would work: case x$offset in (x) deny 'offset is empty' # or rather just skip the -O option ;; (x*[!0-9]*) deny 'offset is negative or contains non-digits' ;; (*) if test ${#offset} -gt 7 || test "$offset" -gt 4194303; then deny 'offset too large' fi ;; esac This code first checks that it’s indeed a positive number, then its length, and only if that’s within safe bounds, the actual value. All of these other than the bc(1)/dc(1) solution, however, impose 32-bit limits on 64-bit platforms. This is, unfortunately, not avoidable because in POSIX sh it’s impossible to find out whether the shell has 32-bit or 64-bit arithmetics without triggering UB on 32-bit. Blaming ISO C is appropriate. I’m attaching a first cut at my favourite solution. It’s missing a thing I’m not sure how to achieve: ensure that bc(1) is available in the initrd. My initramfs-tools “fu” is not very high. Thanks, //mirabilos -- “It is inappropriate to require that a time represented as seconds since the Epoch precisely represent the number of seconds between the referenced time and the Epoch.” -- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2
Bug#994056: cryptsetup: blkid check fails to take offset option into account
Dixi quod… >I’m attaching a first cut at my favourite solution. It’s missing … this time with attachment… bye, //mirabilos -- „Cool, /usr/share/doc/mksh/examples/uhr.gz ist ja ein Grund, mksh auf jedem System zu installieren.“ -- XTaran auf der OpenRheinRuhr, ganz begeistert (EN: “[…]uhr.gz is a reason to install mksh on every system.”)diff --git a/debian/checks/blkid b/debian/checks/blkid index 5332ba35..0be466f1 100644 --- a/debian/checks/blkid +++ b/debian/checks/blkid @@ -17,7 +17,23 @@ dev="$1" fs="$2" offset="$3" -blkid="$(/sbin/blkid -o value -s TYPE -p ${offset:+-O "$((offset*512))"} -- "$dev")" +case x$offset in +(x) + blkid_offset= + ;; +(x*[!0-9]*) + echo " - Invalid 'offset' option: $offset" + exit 1 + ;; +(*) + blkid_offset="-O $(bc <= 2:1.6.0), + bc, dmsetup, ${misc:Depends}, ${shlibs:Depends}
Bug#994056: cryptsetup: blkid check fails to take offset option into account
Package: cryptsetup Version: 2:2.3.5-1 Severity: important X-Debbugs-Cc: t...@mirbsd.de In order to use a cryptsetup swap with a very tiny protective ext2fs filesystem so we can use LABEL= as source device, I use offset= as shown in the Arch Linux wiki. However it fails in Debian: tglase@tglase-nb:~ $ sudo cryptdisks_start cswap Starting crypto disk...cswap (starting)...cswap: the precheck for '/dev/sda2' failed: - The device /dev/sda2 contains a filesystem type ext2. ... (warning). failed. The cause is missing option processing for offset there, with a very simple fix. I have attached a “git diff” against the git tag corresponding to the version in bullseye right now; it applies to the following files “in situ”, in patch order (so people can fix their local systems, even if this is not applied): • /lib/cryptsetup/checks/blkid • /lib/cryptsetup/checks/un_blkid • /lib/cryptsetup/cryptdisks-functions I’m writing this all up as well at: https://evolvis.org/plugins/scmgit/cgi-bin/gitweb.cgi?p=shellsnippets/shellsnippets.git;a=blob;f=posix/swapcycle;hb=HEAD -- Package-specific info: -- /proc/cmdline BOOT_IMAGE=/vmlinuz-5.10.0-8-amd64 root=/dev/sda4 ro rootdelay=5 syscall.x32=y vsyscall=emulate net.ifnames=0 kaslr pcie_aspm=force consoleblank=0 -- /etc/crypttab # cswap LABEL=swp_tglase-nb /dev/random offset=1024,discard,cipher=aes-xts-plain64,size=256,plain,swap -- /etc/fstab /dev/sda4 / ext4 relatime,errors=remount-ro,auto_da_alloc 0 1 /dev/sda1 /boot ext4 noatime,sync,auto_da_alloc 0 2 swap /var/cache/apt tmpfs nosuid,noexec,mode=0755 0 0 /dev/mapper/cswap swapswap sw,discard 0 0 -- lsmod Module Size Used by apple_mfi_fastcharge20480 0 cpuid 16384 0 snd_seq_dummy 16384 0 fuse 167936 2 ctr16384 3 ccm20480 9 cpufreq_conservative16384 0 cpufreq_ondemand 16384 2 cpufreq_userspace 20480 0 cpufreq_powersave 20480 0 binfmt_misc24576 1 nft_counter16384 1 nft_chain_nat 16384 1 xt_MASQUERADE 20480 1 nf_nat 53248 2 nft_chain_nat,xt_MASQUERADE nf_conntrack 176128 2 nf_nat,xt_MASQUERADE nf_defrag_ipv6 24576 1 nf_conntrack nf_defrag_ipv4 16384 1 nf_conntrack nft_compat 20480 1 nf_tables 245760 5 nft_compat,nft_counter,nft_chain_nat x_tables 53248 2 nft_compat,xt_MASQUERADE libcrc32c 16384 3 nf_conntrack,nf_nat,nf_tables nfnetlink 16384 2 nft_compat,nf_tables tun57344 3 snd_seq_midi 20480 0 snd_seq_midi_event 16384 1 snd_seq_midi snd_rawmidi45056 1 snd_seq_midi snd_seq86016 3 snd_seq_midi,snd_seq_midi_event,snd_seq_dummy snd_seq_device 16384 3 snd_seq,snd_seq_midi,snd_rawmidi msr16384 0 ecb16384 1 aes_generic36864 8 libaes 16384 1 aes_generic crypto_simd16384 0 cryptd 24576 1 crypto_simd glue_helper16384 0 xts16384 1 dm_crypt 53248 1 dm_mod163840 2 dm_crypt snd_hda_codec_analog20480 1 snd_hda_codec_generic98304 1 snd_hda_codec_analog iwl4965 110592 0 iwlegacy 90112 1 iwl4965 ppdev 24576 0 snd_hda_intel 57344 0 snd_intel_dspcfg 28672 1 snd_hda_intel pcmcia 77824 0 soundwire_intel45056 1 snd_intel_dspcfg mac80211 983040 2 iwl4965,iwlegacy coretemp 20480 0 soundwire_generic_allocation16384 1 soundwire_intel snd_soc_core 315392 1 soundwire_intel kvm_intel 327680 0 snd_compress 32768 1 snd_soc_core soundwire_cadence 36864 1 soundwire_intel snd_hda_codec 172032 3 snd_hda_codec_generic,snd_hda_intel,snd_hda_codec_analog snd_hda_core 110592 4 snd_hda_codec_generic,snd_hda_intel,snd_hda_codec_analog,snd_hda_codec snd_hwdep 16384 1 snd_hda_codec iTCO_wdt 16384 0 kvm 917504 1 kvm_intel intel_pmc_bxt 16384 1 iTCO_wdt irqbypass 16384 1 kvm soundwire_bus 90112 3 soundwire_intel,soundwire_generic_allocation,soundwire_cadence cfg80211 970752 3 iwl4965,iwlegacy,mac80211 iTCO_vendor_support16384 1 iTCO_wdt serio_raw 20480 0 pcspkr 16384 0 yenta_socket 53248 0 sg 36864 0 pcmcia_rsrc24576 1 yenta_socket snd_pcm_oss65536 0 watchdog 28672 1 iTCO_wdt thinkpad_acpi 118784 0 snd_mixer_oss 28672 1 snd_pcm_oss pcmc