On 09/07/2024 11:41, Alice Ryhl wrote:
On Tue, Jul 9, 2024 at 10:45 AM Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote:
This patch adds a new panic screen, with a QR code and the kmsg data
embedded.
If DRM_PANIC_SCREEN_QR_CODE_URL is set, then the kmsg data will be
compressed with zlib and encoded as a numerical segment, and appended
to the url as a url parameter. This allows to save space, and put
about ~7500 bytes of kmsg data, in a V40 QR code.
Linux distributions can customize the url, and put a web frontend to
directly open a bug report with the kmsg data.
Otherwise the kmsg data will be encoded as binary segment (ie raw
ascii) and only a maximum of 2953 bytes of kmsg data will be
available in the QR code.
You can also limit the QR code size with DRM_PANIC_SCREEN_QR_VERSION.
v2:
* Rewrite the rust comments with Markdown (Alice Ryhl)
* Mark drm_panic_qr_generate() as unsafe (Alice Ryhl)
* Use CStr directly, and remove the call to as_str_unchecked()
(Alice Ryhl)
* Add a check for data_len <= data_size (Greg KH)
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
[...]
+/// drm_panic_qr_generate()
+///
+/// C entry point for the rust QR Code generator.
+///
+/// Write the QR code image in the data buffer, and return the qrcode size, or 0
+/// if the data doesn't fit in a QR code.
+///
+/// * `url` The base url of the QR code. It will be encoded as Binary segment.
+/// * `data` A pointer to the binary data, to be encoded. if url is NULL, it
+/// will be encoded as binary segment, otherwise it will be encoded
+/// efficiently as a numeric segment, and appended to the url.
+/// * `data_len` Length of the data, that needs to be encoded.
+/// * `data_size` Size of data buffer, it should be at least 4071 bytes to hold
+/// a V40 QR-code. It will then be overwritten with the QR-code image.
+/// * `tmp` A temporary buffer that the QR-code encoder will use, to write the
+/// segments and ECC.
+/// * `tmp_size` Size of the temporary buffer, it must be at least 3706 bytes
+/// long for V40.
+///
+/// # Safety
+///
+/// * `url` must be null or point at a nul-terminated string.
+/// * `data` must be valid for reading and writing for `data_size` bytes.
+/// * `data_len` must be less than `data_size`.
+/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
You don't allow data_len == data_size?
In fact, the QR code will always be larger than the data you want to put
into, because it adds segment header/length and ECC, so it doesn't make
sense in practice to have data_len == data_size.
data_size must be at least 4071 bytes, and the maximum data_len you can
put in a V40 is 3703 bytes.
+#[no_mangle]
+pub unsafe extern "C" fn drm_panic_qr_generate(
+ url: *const i8,
+ data: *mut u8,
+ data_len: usize,
+ data_size: usize,
+ tmp: *mut u8,
+ tmp_size: usize,
+) -> u8 {
+ if data_size <= 4071 || tmp_size <= 3706 || data_len > data_size {
+ return 0;
+ }
Since you explicitly check the data_len, it does not *need* to be a
safety requirement (but it can be). Even if it's wrong, violating the
requirement does not lead to memory safety.
Ok, that makes sense, I will move it to the previous section.
+ // Safety: data must be a valid pointer for reading and writing data_size bytes.
+ let data_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(data, data_size) };
+ // Safety: tmp must be a valid pointer for reading and writing tmp_size bytes.
+ let tmp_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(tmp, tmp_size) };
These safety comments explain why these calls are dangerous, but
that's not what safety comments should do. They should explain why
this particular call is okay. In this case, it's because the caller of
drm_panic_qr_generate must follow the documented safety requirements
of the current function. The wording could look like this:
// SAFETY: Due to the safety requirements on this function, the caller
ensures that tmp is a valid pointer for reading and writing tmp_size
bytes.
The wording is not much different, but it's an important distinction.
Ok, I will update it, and add the expected lifetime as Miguel pointed out.
(Also, safety comments are written SAFETY: not Safety:)
+ if url.is_null() {
+ match EncodedMsg::new(&[&Segment::Binary(&data_slice[0..data_len])], tmp_slice) {
+ None => 0,
+ Some(em) => {
+ let qr_image = QrImage::new(&em, data_slice);
+ qr_image.width
+ }
+ }
+ } else {
+ // Safety: url must be a valid pointer to a nul-terminated string.
+ let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) };
// SAFETY: The caller ensures that url is a valid pointer to a
nul-terminated string.
ok
+ let segments = &[
+ &Segment::Binary(url_cstr.as_bytes()),
+ &Segment::Numeric(&data_slice[0..data_len]),
+ ];
+ match EncodedMsg::new(segments, tmp_slice) {
+ None => 0,
+ Some(em) => {
+ let qr_image = QrImage::new(&em, data_slice);
+ qr_image.width
+ }
+ }
+ }
+}
Alice
Best regards,
--
Jocelyn
