linux-kernel - Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080112203848.GA31954@gollum.tnic>
Date:	Sat, 12 Jan 2008 21:38:48 +0100
From:	Borislav Petkov <bbpetkov@...oo.de>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org
Subject: Re: [PATCH 06/21] ide-floppy: remove struct
	idefloppy_flexible_disk_page


[...]

> 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).

upps, sorry, that was silly. I changed it to:

        floppy->wp = !!(pc.buffer[3] & 0x80);

> >  	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).

How about we get rid of that chunk altogether? floppy->flexible_disk_page is
used only here for the purpose of printk-ing to the syslog and has no "real"
purpose otherwise. Do we need that info spewed into the syslog at all?

-- 
Regards/Gruß,
    Boris.
--
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/