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

[Jocelyn Falempe <jfalempe@redhat.com>]

On 11/07/2024 23:00, Alice Ryhl wrote:
> On Wed, Jul 10, 2024 at 4:01 PM Jocelyn Falempe <jfalempe@redhat.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 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@redhat.com>
>> ---
> 
> 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