| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <81B965F9-3A09-40D0-87DF-611A153E744C@getmailspring.com>
Date: Wed, 6 May 2020 03:28:17 -0300
From: "Daniel W. S. Almeida" <dwlsalmeida@...il.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: "sean@...s.org" <sean@...s.org>,
"kstewart@...uxfoundation.org"
<kstewart@...uxfoundation.org>,
"allison@...utok.net" <allison@...utok.net>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>,
"skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
"linux-kernel-mentees@...ts.linuxfoundation.org"
<linux-kernel-mentees@...ts.linuxfoundation.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [RFC, WIP, v4 08/11] media: vidtv: implement a PSI
generator
Hi Mauro,
>> +static u32
>> +vidtv_psi_ts_psi_write_into(struct psi_write_args args)
>> +{
>> + /*
>> + * Packetize PSI sections into TS packets:
>> + * push a TS header (4bytes) every 184 bytes
>> + * manage the continuity_counter
>> + * add stuffing after the CRC
>> + */
>> +
>> + u32 nbytes_past_boundary = (args.dest_offset % TS_PACKET_LEN);
>> + bool aligned = nbytes_past_boundary == 0;
>> +
>> + /*
>> + * whether we need to fragment the data into multiple ts packets
>> + * if we are aligned we need to spare one byte for the pointer_field
>> + */
>> + bool split = (aligned) ?
>> + args.len > TS_PAYLOAD_LEN - 1 :
>> + nbytes_past_boundary + args.len > TS_PACKET_LEN;
>> +
>> + /* how much we can write in this packet */
>> + u32 payload_write_len = (split) ?
>> + (aligned) ? TS_PAYLOAD_LEN :
>> + TS_PACKET_LEN - nbytes_past_boundary :
>> + args.len;
>> +
>> + struct psi_write_args new_args = {0};
>> + struct vidtv_mpeg_ts ts_header = {0};
>> +
>> + u32 nbytes = 0; /* number of bytes written by this function */
>> + u32 temp = 0;
>> +
>> + /* Just a sanity check, should not really happen because we stuff
>> + * the packet when we finish a section, i.e. when we write the crc at
>> + * the end. But if this happens then we have messed up the logic
>> + * somewhere.
>> + */
>> + WARN_ON(args.new_psi_section && !aligned);
>
> Please use a ratelimited printk instead (here and on all similar cases
> at the TS generator).
>
> Also, I think that, on such case, the driver should be filling the
> remaining frame with pad bytes. E. g.:
>
> /*
> * Assuming that vidtv_memset() args from patch 06/11 were changed
> * according with this prototype:
> */
> size_t vidtv_memset(void *to, size_t to_offset, size_t to_size,
> u8 c, size_t len);
>
>
> #define TS_FILL_BYTE 0xff
>
> if (args.new_psi_section && !aligned) {
> pr_warn_ratelimit("Warning: PSI not aligned. Re-aligning it\n");
>
> vidtv_memset(args.dest_buf,
> args.dest_offset + nbytes_past_boundary,
> args.dest_buf_sz,
> TS_FILL_BYTE,
> TS_PACKET_LEN - nbytes_past_boundary);
> args.dest_offset += TS_PACKET_LEN - nbytes_past_boundary;
> aligned = 1;
> nbytes_past_boundary = 0;
> }
>
Sure, that's fine then! Just to be clear this should not happen at all,
because the writes should go through one of these four functions (IIRC!):
u32 vidtv_ts_null_write_into(struct null_packet_write_args args)
u32 vidtv_ts_pcr_write_into(struct pcr_write_args args)
...which will write a single packet at a time, thus leaving the buffer
aligned if it was already aligned to begin with,
and
u32 vidtv_pes_write_into(struct pes_write_args args)
static u32
vidtv_psi_ts_psi_write_into(struct psi_write_args args)
...which will pad when they finish writing a PES packet or a table
section, respectively.
I left these warnings behind as a way to warn me if the padding logic
itself is broken.
> Please use a ratelimited printk instead (here and on all similar cases
> at the TS generator).
OK.
>> +static void vidtv_psi_desc_to_be(struct vidtv_psi_desc *desc)
>> +{
>> + /*
>> + * Convert descriptor endianness to big-endian on a field-by-field
>> basis
>> + * where applicable
>> + */
>> +
>> + switch (desc->type) {
>> + /* nothing do do */
>> + case SERVICE_DESCRIPTOR:
>> + break;
>> + case REGISTRATION_DESCRIPTOR:
>> + cpu_to_be32s(&((struct vidtv_psi_desc_registration *)
>> + desc)->format_identifier);
>> + pr_alert("%s: descriptor type %d found\n",
>> + __func__,
>> + desc->type);
>> + pr_alert("%s: change 'additional_info' endianness before
>> calling\n",
>> + __func__);
>
> The above pr_alert() calls sound weird. Why are you unconditionally
> calling it (and still doing the BE conversion) for all
> REGISTRATION_DESCRIPTOR types?
To be honest, I did not know what to do. Here's what 13818-1 has to say
about registration descriptors:
>2.6.9
> Semantic definition of fields in registration descriptor
>format_identifier – The format_identifier is a 32-bit value obtained
>from a Registration Authority as designated by
>ISO/IEC JTC 1/SC 29.
>additional_identification_info – The meaning of
>additional_identification_info bytes, if any, are defined by the
>assignee of that format_identifier, and once defined they shall not change.
So I took the cue from libdvbv5 and defined the following struct for it,
with a flexible array member at the end:
struct vidtv_psi_desc_registration {
u8 type;
u8 length;
struct vidtv_psi_desc *next;
/*
* The format_identifier is a 32-bit value obtained from a Registration
* Authority as designated by ISO/IEC JTC 1/SC 29.
*/
u32 format_identifier;
/*
* The meaning of additional_identification_info bytes, if any, are
* defined by the assignee of that format_identifier, and once defined
* they shall not change.
*/
u8 additional_identification_info[];
} __packed
As you know, I was changing the endianness from host to BE before
serializing and then changing back from BE to host. Given the struct
definition above, there was no way to do this to the
'additional_identification_info' member, since we do not know its layout.
If we go with your approach instead (i.e. using __be16, __be32...etc)
then I think we can remove these two functions (and more)
thanks,
- Daniel
Powered by blists - more mailing lists