| Message ID | 20181104134234.13597-1-gmazyland@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | [RFC] dm: Check for device sector overflow if CONFIG_LBDAF is not set | expand |
On Sun, 4 Nov 2018, Milan Broz wrote: > Reference to a device in device-mapper table contains offset in sectors. > > If the sector_t is 32bit integer (CONFIG_LBDAF is not set), then > several device-mapper targets can overflow this offset and validity > check is then performad on wrong offset and wrong table is activated. > > See for example (on 32bit without CONFIG_LBDAF) this overflow: > > # dmsetup create test --table "0 2048 linear /dev/sdg 4294967297" > # dmsetup table test > 0 2048 linear 8:96 1 > > In this patch I tried to add check for this problem to dm-linear and dm-crypt, > but I am sure there are more places and I am not sure this is the proper way. > > Should we use uint64_t in DM internally for device offset instead? > > There are probably some internal calculations in dm-table.c that > can overflow as well. > > NOTE: it is a RFC patch that is incomplete (more targets need fixes). OK. But the condition "sizeof(cc->start) < sizeof(tmpll)" could be dropped, the compiler will optimize out "cc->start != tmpll" if the types have the same width. Mikulas > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > drivers/md/dm-crypt.c | 4 ++++ > drivers/md/dm-linear.c | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 49be7a6a2e81..008fc40ef84b 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -2786,6 +2786,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) > goto bad; > } > cc->start = tmpll; > + if (sizeof(cc->start) < sizeof(tmpll) && cc->start != tmpll) { > + ti->error = "Device sector overflow"; > + goto bad; > + } > > if (crypt_integrity_aead(cc) || cc->integrity_iv_size) { > ret = crypt_integrity_ctr(cc, ti); > diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c > index 8d7ddee6ac4d..b5a0065d1436 100644 > --- a/drivers/md/dm-linear.c > +++ b/drivers/md/dm-linear.c > @@ -50,6 +50,10 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv) > goto bad; > } > lc->start = tmp; > + if (sizeof(lc->start) < sizeof(tmp) && lc->start != tmp) { > + ti->error = "Device sector overflow"; > + goto bad; > + } > > ret = dm_get_device(ti, argv[0], dm_table_get_mode(ti->table), &lc->dev); > if (ret) { > -- > 2.19.1 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 05/11/2018 19:35, Mikulas Patocka wrote: > But the condition "sizeof(cc->start) < sizeof(tmpll)" could be dropped, > the compiler will optimize out "cc->start != tmpll" if the types have the > same width. Yes, the intention here is that in 64bit env. the whole if condition is not compiled in. If it happens without "sizeof(cc->start) < sizeof(tmpll)", then we can drop it. So, does it make sense to add this to all dm targets? Or any better idea? Milan -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 5 Nov 2018, Milan Broz wrote: > On 05/11/2018 19:35, Mikulas Patocka wrote: > > But the condition "sizeof(cc->start) < sizeof(tmpll)" could be dropped, > > the compiler will optimize out "cc->start != tmpll" if the types have the > > same width. > > Yes, the intention here is that in 64bit env. the whole if condition > is not compiled in. If it happens without "sizeof(cc->start) < sizeof(tmpll)", > then we can drop it. > > So, does it make sense to add this to all dm targets? Or any better idea? > > Milan In the targets that I have written, I tried to protect against this: if (sscanf(argv[1], "%llu%c", &start, &dummy) != 1 || start != (sector_t)start) ic->provided_data_sectors = le64_to_cpu(ic->sb->provided_data_sectors) if (ic->provided_data_sectors != le64_to_cpu(ic->sb->provided_data_sectors)) { wc->start_sector = start_sector; if (wc->start_sector != start_sector If you find some more unprotected cases, it's OK to convert them. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c index 49be7a6a2e81..008fc40ef84b 100644 --- a/drivers/md/dm-crypt.c +++ b/drivers/md/dm-crypt.c @@ -2786,6 +2786,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) goto bad; } cc->start = tmpll; + if (sizeof(cc->start) < sizeof(tmpll) && cc->start != tmpll) { + ti->error = "Device sector overflow"; + goto bad; + } if (crypt_integrity_aead(cc) || cc->integrity_iv_size) { ret = crypt_integrity_ctr(cc, ti); diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c index 8d7ddee6ac4d..b5a0065d1436 100644 --- a/drivers/md/dm-linear.c +++ b/drivers/md/dm-linear.c @@ -50,6 +50,10 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv) goto bad; } lc->start = tmp; + if (sizeof(lc->start) < sizeof(tmp) && lc->start != tmp) { + ti->error = "Device sector overflow"; + goto bad; + } ret = dm_get_device(ti, argv[0], dm_table_get_mode(ti->table), &lc->dev); if (ret) {
Reference to a device in device-mapper table contains offset in sectors. If the sector_t is 32bit integer (CONFIG_LBDAF is not set), then several device-mapper targets can overflow this offset and validity check is then performad on wrong offset and wrong table is activated. See for example (on 32bit without CONFIG_LBDAF) this overflow: # dmsetup create test --table "0 2048 linear /dev/sdg 4294967297" # dmsetup table test 0 2048 linear 8:96 1 In this patch I tried to add check for this problem to dm-linear and dm-crypt, but I am sure there are more places and I am not sure this is the proper way. Should we use uint64_t in DM internally for device offset instead? There are probably some internal calculations in dm-table.c that can overflow as well. NOTE: it is a RFC patch that is incomplete (more targets need fixes). Signed-off-by: Milan Broz <gmazyland@gmail.com> --- drivers/md/dm-crypt.c | 4 ++++ drivers/md/dm-linear.c | 4 ++++ 2 files changed, 8 insertions(+)