On Thu, May 06, 2010 at 02:52:22PM -0000, Ang Way Chuang wrote:
> ULE (Unidirectional Lightweight Encapsulation RFC 4326) decapsulation
> code has a bug that incorrectly treats ULE SNDU packed into the
> remaining 2 or 3 bytes of a MPEG2-TS frame as having invalid pointer
> field on the subsequent MPEG2-TS frame.
>
> This patch was generated and tested against v2.6.34-rc6. I suspect
> that this bug was introduced in kernel version 2.6.15, but had not
> verified it.
>
> Care has been taken not to introduce more bug by fixing this bug, but
> please scrutinize the code because I always produces buggy code.
>
> Signed-off-by: Ang Way Chuang <[email protected]>
>
> ---
>
> diff --git a/drivers/media/dvb/dvb-core/dvb_net.c
> b/drivers/media/dvb/dvb-core/dvb_net.c
> index 441c064..35a4afb 100644
> --- a/drivers/media/dvb/dvb-core/dvb_net.c
> +++ b/drivers/media/dvb/dvb-core/dvb_net.c
> @@ -458,8 +458,9 @@ static void dvb_net_ule( struct net_device *dev, const u8
> *buf, size_t buf_len )
> "field: %u.\n",
> priv->ts_count, *from_where);
>
> /* Drop partly decoded SNDU,
> reset state, resync on PUSI. */
> - if (priv->ule_skb) {
> - dev_kfree_skb(
> priv->ule_skb );
> + if (priv->ule_skb ||
> priv->ule_sndu_remain) {
> + if (priv->ule_skb)
> + dev_kfree_skb(
> priv->ule_skb );
> dev->stats.rx_errors++;
>
> dev->stats.rx_frame_errors++;
> }
That code block looks odd that way, but after staring at it for a minute,
it makes sense. Another way to do it that might read cleaner (and reduce
excessive tab indent levels) would be to add a 'bool errors', then:
bool errors = false;
...
if (priv->ule_skb) {
errors = true;
dev_kfree_skb(priv->ule_skb);
}
if (errors || priv->ule_sndu_remain) {
dev->stats.rx_errors++;
dev->stats.rx_frame_errors++;
}
> @@ -534,6 +535,7 @@ static void dvb_net_ule( struct net_device *dev, const u8
> *buf, size_t buf_len )
> from_where += 2;
> }
>
> + priv->ule_sndu_remain = priv->ule_sndu_len + 2;
> /*
> * State of current TS:
> * ts_remain (remaining bytes in the current TS cell)
Is this *always* true? Your description says "...the remaining 2 or 3
bytes", indicating this could sometimes need to be +3. Is +0 also a
possibility?
> @@ -543,6 +545,7 @@ static void dvb_net_ule( struct net_device *dev, const u8
> *buf, size_t buf_len )
> */
> switch (ts_remain) {
> case 1:
> + priv->ule_sndu_remain--;
> priv->ule_sndu_type = from_where[0] <<
> 8;
> priv->ule_sndu_type_1 = 1; /* first
> byte of ule_type is set. */
> ts_remain -= 1; from_where += 1;
> @@ -556,6 +559,7 @@ static void dvb_net_ule( struct net_device *dev, const u8
> *buf, size_t buf_len )
> default: /* complete ULE header is present in
> current TS. */
> /* Extract ULE type field. */
> if (priv->ule_sndu_type_1) {
> + priv->ule_sndu_type_1 = 0;
> priv->ule_sndu_type |=
> from_where[0];
> from_where += 1; /* points to
> payload start. */
> ts_remain -= 1;
--
Jarod Wilson
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
