Message-ID: <CAH5fLgh5fb_NYUNPPXYepJg=pbmHAb+-+sOrCxc0n=fiNjTFTw@mail.gmail.com>
Date: Tue, 9 Jul 2024 11:41:29 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Jocelyn Falempe <jfalempe@...hat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Bjorn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...sung.com>, linux-kernel@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org, 
	Danilo Krummrich <dakr@...hat.com>
Subject: Re: [PATCH v2 4/4] drm/panic: Add a qr_code panic screen

On Tue, Jul 9, 2024 at 10:45 AM Jocelyn Falempe <jfalempe@...hat.com> 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@...hat.com>

[...]

> +/// 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?

> +#[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.

> +    // 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.

(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.

> +        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

Powered by blists - more mailing lists