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
