Re: [PATCH v2 4/4] drm/panic: Add a qr_code panic screen

[Jocelyn Falempe <jfalempe@redhat.com>]

On 09/07/2024 11:41, Alice Ryhl wrote:
> On Tue, Jul 9, 2024 at 10:45 AM 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 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@redhat.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?

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