| Message ID | 20180214102850.28755-3-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 14 February 2018 11:28:46 Jan Kara wrote: > According to ECMA-167 3/8.4.2 Volume Descriptor Pointer is terminating > current extent of Volume Descriptor Sequence. Also according to ECMA-167 > 3/8.4.3 Volume Descriptor Sequence Number is not significant for Volume > Descriptor Pointers. Simplify the handling of Volume Descriptor Pointers > to take this into account. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/udf/super.c | 41 ++++++++++++++++------------------------- > 1 file changed, 16 insertions(+), 25 deletions(-) > > diff --git a/fs/udf/super.c b/fs/udf/super.c > index 5c5d5fd513cc..f80b97173acd 100644 > --- a/fs/udf/super.c > +++ b/fs/udf/super.c > @@ -1615,7 +1615,6 @@ static noinline int udf_process_sequence( > bool done = false; > uint32_t vdsn; > uint16_t ident; > - long next_s = 0, next_e = 0; > int ret; > unsigned int indirections = 0; > > @@ -1647,19 +1646,22 @@ static noinline int udf_process_sequence( > } > break; > case TAG_IDENT_VDP: /* ISO 13346 3/10.3 */ > - curr = &vds[VDS_POS_VOL_DESC_PTR]; > - if (vdsn >= curr->volDescSeqNum) { > - curr->volDescSeqNum = vdsn; > - curr->block = block; > - > - vdp = (struct volDescPtr *)bh->b_data; > - next_s = le32_to_cpu( > - vdp->nextVolDescSeqExt.extLocation); > - next_e = le32_to_cpu( > - vdp->nextVolDescSeqExt.extLength); > - next_e = next_e >> sb->s_blocksize_bits; > - next_e += next_s - 1; > + if (++indirections > UDF_MAX_TD_NESTING) { > + udf_err(sb, "too many Volume Descriptor " > + "Pointers (max %u supported)\n", > + UDF_MAX_TD_NESTING); > + brelse(bh); > + return -EIO; > } > + > + vdp = (struct volDescPtr *)bh->b_data; > + block = le32_to_cpu(vdp->nextVolDescSeqExt.extLocation); Another pathological case: disc with two (or more) VDP descriptors and each points to another in cycle. Seems that this would not cause infinite loop due to UDF_MAX_TD_NESTING, but probably can cause some troubles. > + lastblock = le32_to_cpu( > + vdp->nextVolDescSeqExt.extLength) >> > + sb->s_blocksize_bits; > + lastblock += block - 1; > + /* For loop is going to increment 'block' again */ > + block--; > break; > case TAG_IDENT_IUVD: /* ISO 13346 3/10.4 */ > curr = &vds[VDS_POS_IMP_USE_VOL_DESC]; > @@ -1688,19 +1690,8 @@ static noinline int udf_process_sequence( > } > break; > case TAG_IDENT_TD: /* ISO 13346 3/10.9 */ > - if (++indirections > UDF_MAX_TD_NESTING) { > - udf_err(sb, "too many TDs (max %u supported)\n", UDF_MAX_TD_NESTING); > - brelse(bh); > - return -EIO; > - } > - > vds[VDS_POS_TERMINATING_DESC].block = block; > - if (next_e) { > - block = next_s; > - lastblock = next_e; > - next_s = next_e = 0; > - } else > - done = true; > + done = true; > break; > } > brelse(bh);
On Wed 14-02-18 18:26:03, Pali Rohár wrote: > On Wednesday 14 February 2018 11:28:46 Jan Kara wrote: > > According to ECMA-167 3/8.4.2 Volume Descriptor Pointer is terminating > > current extent of Volume Descriptor Sequence. Also according to ECMA-167 > > 3/8.4.3 Volume Descriptor Sequence Number is not significant for Volume > > Descriptor Pointers. Simplify the handling of Volume Descriptor Pointers > > to take this into account. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/udf/super.c | 41 ++++++++++++++++------------------------- > > 1 file changed, 16 insertions(+), 25 deletions(-) > > > > diff --git a/fs/udf/super.c b/fs/udf/super.c > > index 5c5d5fd513cc..f80b97173acd 100644 > > --- a/fs/udf/super.c > > +++ b/fs/udf/super.c > > @@ -1615,7 +1615,6 @@ static noinline int udf_process_sequence( > > bool done = false; > > uint32_t vdsn; > > uint16_t ident; > > - long next_s = 0, next_e = 0; > > int ret; > > unsigned int indirections = 0; > > > > @@ -1647,19 +1646,22 @@ static noinline int udf_process_sequence( > > } > > break; > > case TAG_IDENT_VDP: /* ISO 13346 3/10.3 */ > > - curr = &vds[VDS_POS_VOL_DESC_PTR]; > > - if (vdsn >= curr->volDescSeqNum) { > > - curr->volDescSeqNum = vdsn; > > - curr->block = block; > > - > > - vdp = (struct volDescPtr *)bh->b_data; > > - next_s = le32_to_cpu( > > - vdp->nextVolDescSeqExt.extLocation); > > - next_e = le32_to_cpu( > > - vdp->nextVolDescSeqExt.extLength); > > - next_e = next_e >> sb->s_blocksize_bits; > > - next_e += next_s - 1; > > + if (++indirections > UDF_MAX_TD_NESTING) { > > + udf_err(sb, "too many Volume Descriptor " > > + "Pointers (max %u supported)\n", > > + UDF_MAX_TD_NESTING); > > + brelse(bh); > > + return -EIO; > > } > > + > > + vdp = (struct volDescPtr *)bh->b_data; > > + block = le32_to_cpu(vdp->nextVolDescSeqExt.extLocation); > > Another pathological case: disc with two (or more) VDP descriptors and > each points to another in cycle. > > Seems that this would not cause infinite loop due to > UDF_MAX_TD_NESTING, but probably can cause some troubles. Yes. Such disk is invalid so our only goal is not to crash / livelock the kernel and UDF_MAX_TD_NESTING protection is enough for that. Honza
On Thursday 15 February 2018 09:43:12 Jan Kara wrote: > On Wed 14-02-18 18:26:03, Pali Rohár wrote: > > On Wednesday 14 February 2018 11:28:46 Jan Kara wrote: > > > According to ECMA-167 3/8.4.2 Volume Descriptor Pointer is terminating > > > current extent of Volume Descriptor Sequence. Also according to ECMA-167 > > > 3/8.4.3 Volume Descriptor Sequence Number is not significant for Volume > > > Descriptor Pointers. Simplify the handling of Volume Descriptor Pointers > > > to take this into account. > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/udf/super.c | 41 ++++++++++++++++------------------------- > > > 1 file changed, 16 insertions(+), 25 deletions(-) > > > > > > diff --git a/fs/udf/super.c b/fs/udf/super.c > > > index 5c5d5fd513cc..f80b97173acd 100644 > > > --- a/fs/udf/super.c > > > +++ b/fs/udf/super.c > > > @@ -1615,7 +1615,6 @@ static noinline int udf_process_sequence( > > > bool done = false; > > > uint32_t vdsn; > > > uint16_t ident; > > > - long next_s = 0, next_e = 0; > > > int ret; > > > unsigned int indirections = 0; > > > > > > @@ -1647,19 +1646,22 @@ static noinline int udf_process_sequence( > > > } > > > break; > > > case TAG_IDENT_VDP: /* ISO 13346 3/10.3 */ > > > - curr = &vds[VDS_POS_VOL_DESC_PTR]; > > > - if (vdsn >= curr->volDescSeqNum) { > > > - curr->volDescSeqNum = vdsn; > > > - curr->block = block; > > > - > > > - vdp = (struct volDescPtr *)bh->b_data; > > > - next_s = le32_to_cpu( > > > - vdp->nextVolDescSeqExt.extLocation); > > > - next_e = le32_to_cpu( > > > - vdp->nextVolDescSeqExt.extLength); > > > - next_e = next_e >> sb->s_blocksize_bits; > > > - next_e += next_s - 1; > > > + if (++indirections > UDF_MAX_TD_NESTING) { > > > + udf_err(sb, "too many Volume Descriptor " > > > + "Pointers (max %u supported)\n", > > > + UDF_MAX_TD_NESTING); > > > + brelse(bh); > > > + return -EIO; > > > } > > > + > > > + vdp = (struct volDescPtr *)bh->b_data; > > > + block = le32_to_cpu(vdp->nextVolDescSeqExt.extLocation); > > > > Another pathological case: disc with two (or more) VDP descriptors and > > each points to another in cycle. > > > > Seems that this would not cause infinite loop due to > > UDF_MAX_TD_NESTING, but probably can cause some troubles. > > Yes. Such disk is invalid so our only goal is not to crash / livelock the > kernel and UDF_MAX_TD_NESTING protection is enough for that. Ok, then you can add my Acked-by on whole patch series. Acked-by: Pali Rohár <pali.rohar@gmail.com>
On Thu 15-02-18 23:33:16, Pali Rohár wrote: > On Thursday 15 February 2018 09:43:12 Jan Kara wrote: > > On Wed 14-02-18 18:26:03, Pali Rohár wrote: > > > On Wednesday 14 February 2018 11:28:46 Jan Kara wrote: > > > > According to ECMA-167 3/8.4.2 Volume Descriptor Pointer is terminating > > > > current extent of Volume Descriptor Sequence. Also according to ECMA-167 > > > > 3/8.4.3 Volume Descriptor Sequence Number is not significant for Volume > > > > Descriptor Pointers. Simplify the handling of Volume Descriptor Pointers > > > > to take this into account. > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > fs/udf/super.c | 41 ++++++++++++++++------------------------- > > > > 1 file changed, 16 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/fs/udf/super.c b/fs/udf/super.c > > > > index 5c5d5fd513cc..f80b97173acd 100644 > > > > --- a/fs/udf/super.c > > > > +++ b/fs/udf/super.c > > > > @@ -1615,7 +1615,6 @@ static noinline int udf_process_sequence( > > > > bool done = false; > > > > uint32_t vdsn; > > > > uint16_t ident; > > > > - long next_s = 0, next_e = 0; > > > > int ret; > > > > unsigned int indirections = 0; > > > > > > > > @@ -1647,19 +1646,22 @@ static noinline int udf_process_sequence( > > > > } > > > > break; > > > > case TAG_IDENT_VDP: /* ISO 13346 3/10.3 */ > > > > - curr = &vds[VDS_POS_VOL_DESC_PTR]; > > > > - if (vdsn >= curr->volDescSeqNum) { > > > > - curr->volDescSeqNum = vdsn; > > > > - curr->block = block; > > > > - > > > > - vdp = (struct volDescPtr *)bh->b_data; > > > > - next_s = le32_to_cpu( > > > > - vdp->nextVolDescSeqExt.extLocation); > > > > - next_e = le32_to_cpu( > > > > - vdp->nextVolDescSeqExt.extLength); > > > > - next_e = next_e >> sb->s_blocksize_bits; > > > > - next_e += next_s - 1; > > > > + if (++indirections > UDF_MAX_TD_NESTING) { > > > > + udf_err(sb, "too many Volume Descriptor " > > > > + "Pointers (max %u supported)\n", > > > > + UDF_MAX_TD_NESTING); > > > > + brelse(bh); > > > > + return -EIO; > > > > } > > > > + > > > > + vdp = (struct volDescPtr *)bh->b_data; > > > > + block = le32_to_cpu(vdp->nextVolDescSeqExt.extLocation); > > > > > > Another pathological case: disc with two (or more) VDP descriptors and > > > each points to another in cycle. > > > > > > Seems that this would not cause infinite loop due to > > > UDF_MAX_TD_NESTING, but probably can cause some troubles. > > > > Yes. Such disk is invalid so our only goal is not to crash / livelock the > > kernel and UDF_MAX_TD_NESTING protection is enough for that. > > Ok, then you can add my Acked-by on whole patch series. > > Acked-by: Pali Rohár <pali.rohar@gmail.com> Thanks for review! Honza
diff --git a/fs/udf/super.c b/fs/udf/super.c index 5c5d5fd513cc..f80b97173acd 100644 --- a/fs/udf/super.c +++ b/fs/udf/super.c @@ -1615,7 +1615,6 @@ static noinline int udf_process_sequence( bool done = false; uint32_t vdsn; uint16_t ident; - long next_s = 0, next_e = 0; int ret; unsigned int indirections = 0; @@ -1647,19 +1646,22 @@ static noinline int udf_process_sequence( } break; case TAG_IDENT_VDP: /* ISO 13346 3/10.3 */ - curr = &vds[VDS_POS_VOL_DESC_PTR]; - if (vdsn >= curr->volDescSeqNum) { - curr->volDescSeqNum = vdsn; - curr->block = block; - - vdp = (struct volDescPtr *)bh->b_data; - next_s = le32_to_cpu( - vdp->nextVolDescSeqExt.extLocation); - next_e = le32_to_cpu( - vdp->nextVolDescSeqExt.extLength); - next_e = next_e >> sb->s_blocksize_bits; - next_e += next_s - 1; + if (++indirections > UDF_MAX_TD_NESTING) { + udf_err(sb, "too many Volume Descriptor " + "Pointers (max %u supported)\n", + UDF_MAX_TD_NESTING); + brelse(bh); + return -EIO; } + + vdp = (struct volDescPtr *)bh->b_data; + block = le32_to_cpu(vdp->nextVolDescSeqExt.extLocation); + lastblock = le32_to_cpu( + vdp->nextVolDescSeqExt.extLength) >> + sb->s_blocksize_bits; + lastblock += block - 1; + /* For loop is going to increment 'block' again */ + block--; break; case TAG_IDENT_IUVD: /* ISO 13346 3/10.4 */ curr = &vds[VDS_POS_IMP_USE_VOL_DESC]; @@ -1688,19 +1690,8 @@ static noinline int udf_process_sequence( } break; case TAG_IDENT_TD: /* ISO 13346 3/10.9 */ - if (++indirections > UDF_MAX_TD_NESTING) { - udf_err(sb, "too many TDs (max %u supported)\n", UDF_MAX_TD_NESTING); - brelse(bh); - return -EIO; - } - vds[VDS_POS_TERMINATING_DESC].block = block; - if (next_e) { - block = next_s; - lastblock = next_e; - next_s = next_e = 0; - } else - done = true; + done = true; break; } brelse(bh);
According to ECMA-167 3/8.4.2 Volume Descriptor Pointer is terminating current extent of Volume Descriptor Sequence. Also according to ECMA-167 3/8.4.3 Volume Descriptor Sequence Number is not significant for Volume Descriptor Pointers. Simplify the handling of Volume Descriptor Pointers to take this into account. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/udf/super.c | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-)