Re: [PATCH v3 4/4] drm/panic: Add a QR code panic screen

[Jocelyn Falempe]

On 11/07/2024 23:00, Alice Ryhl wrote:
> On Wed, Jul 10, 2024 at 4:01 PM 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 a 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)
> >
> > v3:
> >   * Fix all rust comments (typo, punctuation) (Miguel Ojeda)
> >   * Change the wording of safety comments (Alice Ryhl)
> >   * Add a link to the javascript decoder in the Kconfig (Greg KH)
> >   * Fix data_size and tmp_size check in drm_panic_qr_generate()
> >
> > Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
> > ---
>
> Overall, it looks reasonable to me. Some comments below.
>
> The changelog should go below the --- or in the cover letter.
>
> > +       if (stream.total_out > max_qr_data_size) {
> > +               /* too much data for the QR code, so skip the first line and try again */
> > +               kmsg = strchr(kmsg + 1, '\n');
> > +               if (!kmsg)
> > +                       return -EINVAL;
> > +               kmsg_len = strlen(kmsg);
> > +               goto try_again;
>
> It seems like kmsg will start with a newline character when this retry
> routine runs. Is that really what you want? Why not instead strchr and
> then do the plus one afterwards?

Good catch, yes I should increment kmsg after the strchr, to skip the 
'\n' character.
> This would also simplify the logic for why `kmsg + 1` doesn't go out
> of bounds. Right now I have to check that there's no codepath where
> kmsg points at the nul terminator byte.

sure, I will do that for v4.

> > +const __LOG_PREFIX: &[u8] = b"rust_qrcode\0";
>
> I guess this constant is because you're not using the module! macro?

I think it's a leftover of when I used pr_info!() to debug my rust code.
Drm panic is built within the main drm module, and is not a module on 
its own, so I'm not sure if I can use the module! macro here.

> > +/// C entry point for the rust QR Code generator.
> > +///
> > +/// Write the QR code image in the data buffer, and return the QR code width,
> > +/// 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, must be less
> > +///    than data_size.
> > +/// * `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.
> > +/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
> > +///
> > +/// They must remain valid for the duration of the function call.
> > +
> > +#[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;
> > +    }
> > +    // SAFETY: The caller ensures that `data` is 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: The caller ensures that `tmp` is 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) };
> > +    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: The caller ensures that `url` is a valid pointer to a
> > +        // nul-terminated string.
> > +        let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) };
> > +        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
> > +            }
> > +        }
> > +    }
> > +}
>
> This looks good to me. :)

Thanks a lot
> Alice


--

Jocelyn