| 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: <200801120158.57895.bzolnier@gmail.com>
Date: Sat, 12 Jan 2008 01:58:57 +0100
From: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To: Borislav Petkov <bbpetkov@...oo.de>
Cc: linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org
Subject: Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
On Friday 11 January 2008, Borislav Petkov wrote:
> The driver used to test whether the flexible disk page has changed by memcmp-ing
> it with a cached copy of a previous version of the page from a different remo-
> vable medium. Since, according to the SFF-8070i spec, the flexible disk page
> "specifies parameters relating to the currently installed medium type," this
> comparison is now done by simply checking whether the medium has changed.
>
> Signed-off-by: Borislav Petkov <bbpetkov@...oo.de>
> ---
> drivers/ide/ide-floppy.c | 89 ++++++++++++++++-----------------------------
> 1 files changed, 32 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index 2b9885f..679d48e 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
[...]
> @@ -1188,50 +1159,54 @@ static int idefloppy_queue_pc_tail (ide_drive_t *drive,idefloppy_pc_t *pc)
> }
>
> /*
> - * Look at the flexible disk page parameters. We will ignore the CHS
> - * capacity parameters and use the LBA parameters instead.
> + * Look at the flexible disk page parameters. We will ignore the CHS capacity
> + * parameters and use the LBA parameters instead.
> */
> -static int idefloppy_get_flexible_disk_page (ide_drive_t *drive)
> +static int idefloppy_get_flexible_disk_page(ide_drive_t *drive)
Care to rename it to ide_floppy_get_flexible_disk_page() while at it
(it has only one user)?
> {
> idefloppy_floppy_t *floppy = drive->driver_data;
> idefloppy_pc_t pc;
> - idefloppy_mode_parameter_header_t *header;
> - idefloppy_flexible_disk_page_t *page;
> int capacity, lba_capacity;
> + u8 heads, sectors;
> + u16 transfer_rate, sector_size, cyls, rpm;
some CodingStyle nitpicks (not really that important, rather personal taste):
u16 transfer_rate, sector_size, cyls, rpm;
u8 heads, sectors;
> - idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE, MODE_SENSE_CURRENT);
> - if (idefloppy_queue_pc_tail(drive,&pc)) {
> - printk(KERN_ERR "ide-floppy: Can't get flexible disk "
> - "page parameters\n");
> + idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
> + MODE_SENSE_CURRENT);
idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
MODE_SENSE_CURRENT);
> + if (idefloppy_queue_pc_tail(drive, &pc)) {
> + printk(KERN_ERR "ide-floppy: Can't get flexible disk page"
> + " parameters\n");
> return 1;
> }
> - header = (idefloppy_mode_parameter_header_t *) pc.buffer;
> - floppy->wp = header->wp;
> + floppy->wp = pc.buffer[3] & 0x80;
This is not an equivalent transformation:
header->wp is 0 or 1
pc.buffer[3] & 0x80 is 0 or 0x80
It seems to work fine for ->wp (because it is needlessly defined as 'int')
but may seriously confuse set_disk_ro() and thus bdev_read_only() users.
Should be fixed to '(pc.buffer[3] & 0x80) ? 1 : 0' (or something similar).
> set_disk_ro(floppy->disk, floppy->wp);
> - page = (idefloppy_flexible_disk_page_t *) (header + 1);
> -
> - page->transfer_rate = be16_to_cpu(page->transfer_rate);
> - page->sector_size = be16_to_cpu(page->sector_size);
> - page->cyls = be16_to_cpu(page->cyls);
> - page->rpm = be16_to_cpu(page->rpm);
> - capacity = page->cyls * page->heads * page->sectors * page->sector_size;
> - if (memcmp (page, &floppy->flexible_disk_page, sizeof (idefloppy_flexible_disk_page_t)))
> +
> + transfer_rate = be16_to_cpu(*(u16 *)&pc.buffer[8 + 2]);
> + sector_size = be16_to_cpu(*(u16 *)&pc.buffer[8 + 6]);
> + cyls = be16_to_cpu(*(u16 *)&pc.buffer[8 + 8]);
> + rpm = be16_to_cpu(*(u16 *)&pc.buffer[8 + 28]);
> + heads = pc.buffer[8 + 4];
> + sectors = pc.buffer[8 + 5];
> +
> + capacity = cyls * heads * sectors * sector_size;
> +
> + if ((1UL << IDEFLOPPY_MEDIA_CHANGED) & floppy->flags)
IDEFLOPPY_MEDIA_CHANGED is set when block device is opened for the first
time (please check idefloppy_open() for details) so I don't think it is
the right change. 'Flexible Disk Page' is only 32 bytes so we are better
off with leaving 'u8 flexible_disk_page[32]' in idefloppy_floppy_t and
doing things the old way.
Besides please do not intermix real changes like the above one with purely
cleanup ones like idefloppy_flexible_disk_page_t removal. This is bad from
maintainability point of view. If some patch causes problems it is easier
to narrow it down by heaving purely cleanup changes separated out + if we
would need to revert the real change we would have to make a separate patch
doing it instead of just reverting the guilty commit (given that we don't
want cleanup changes to be reverted as well).
> printk(KERN_INFO "%s: %dkB, %d/%d/%d CHS, %d kBps, "
> "%d sector size, %d rpm\n",
> - drive->name, capacity / 1024, page->cyls,
> - page->heads, page->sectors,
> - page->transfer_rate / 8, page->sector_size, page->rpm);
> -
> - floppy->flexible_disk_page = *page;
> - drive->bios_cyl = page->cyls;
> - drive->bios_head = page->heads;
> - drive->bios_sect = page->sectors;
> + drive->name, capacity / 1024, cyls, heads, sectors,
> + transfer_rate / 8, sector_size, rpm);
more CodingStyle nitpicks:
printk(KERN_INFO "%s: %dkB, %d/%d/%d CHS, %d kBps, "
"%d sector size, %d rpm\n",
drive->name, capacity / 1024, cyls, heads, sectors,
transfer_rate / 8, sector_size, rpm);
would be more readable IMO
> + drive->bios_cyl = cyls;
> + drive->bios_head = heads;
> + drive->bios_sect = sectors;
extra newline would distinguish the above block from the code below
> lba_capacity = floppy->blocks * floppy->block_size;
> +
> if (capacity < lba_capacity) {
> printk(KERN_NOTICE "%s: The disk reports a capacity of %d "
> "bytes, but the drive only handles %d\n",
> drive->name, lba_capacity, capacity);
> - floppy->blocks = floppy->block_size ? capacity / floppy->block_size : 0;
> + floppy->blocks = floppy->block_size ?
> + capacity / floppy->block_size : 0;
> }
> return 0;
> }
Otherwise looks fine, please recast and resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists