On 2018年06月08日 03:57, james harvey wrote: > "On Thu, Jun 7, 2018 at 3:26 AM, Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote: >> On 2018年06月07日 15:19, james harvey wrote: >>> On Thu, Jun 7, 2018 at 12:44 AM, Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote: >>>> On 2018年06月07日 11:33, james harvey wrote: >>>>> * btrfs_item_key_to_cpu sets key to (10955960320, BTRFS_EXTENT_ITEM_KEY, 4096) >>>>> !!! Bonus bug, LEFT FOR READER. Why is this item #197, 5 items before the 203 >>>>> given? I think no bounds checking causes a buffer over-read here. >>>>> btrfs_item_key_to_cpu() calls btrfs_item_key(), which uses the macro >>>>> read_eb_member() to call read_extent_buffer() which memcpy's using an out >>>>> of range index, at least for this leaf. >>>> >>>> Here the key you got could be garbage. >>>> Either the leaf has enough free space, then you got pending zero into >>>> the key, or the leaf is full, you got some item data into the key. >>>> >>>> Either way, the key should be read/trusted at all. >>> >>> Is the group's preference to leave things like bounds checking to the >>> caller? >> >> The problem here is, btrfs_search_slot() is not only called for >> read_only search, but also for insert. >> >> For insert usage, such returned path is completely valid and in fact >> could be pretty useful. > > I don't mean for btrfs_search_slot() behavior to change, since it is > called for inserting. I mean btrfs_item_key_to_cpu() behavior could > change. Makes sense! An nice point to enhance! > >>> (I get the merit to that, avoiding redundant checks.) My >>> normal style would be to have read_extent_buffer() fail if start + len >>> is out of bounds. >> >> In this case, it's not out-of-boundary at all, and the invalid key you >> read out if from the last item, which could contain a valid key. >> >> For leaf, btrfs puts btrfs items pointers at the head, and the items >> data at the tail. >> >> Thus for this case, your invalid key slot is just in the middle of the >> leaf, making read_extent_buffer() unable to detect anything wrong. >> >> Thanks, >> Qu > > You're right. read_extent_buffer() doesn't have the information to > look at this. > > And, "buffer over-read error" was probably the wrong term. I mean in > the sense that it has 203 valid values, and is being asked for the > 204'th item. > > btrfs_item_key() could error on > > (nr > btrfs_header_nritems(eb)) > > This defensive programming would cause redundant checks, if the caller > checks this too as they are supposed to, so I understand if the group > doesn't swing that far on defensive programming. > > Although, perhaps in such situation, btrfs_item_key() could > automatically call btrfs_next_leaf(). This introduce extra return value, and for things like btrfs_item_key(), we need to modify tons of call sites. But at least, I could check if doing extra check (mostly a WARN_ON()) in btrfs_item_key() is valid. Thanks, Qu > I'm thinking this might not be > desired behavior everywhere, but just in case it is, mentioning it > because then btrfs_item_key() could handle all of this, not allowing > the caller to shoot their own foot, without redundant checks. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Attachment:
signature.asc
Description: OpenPGP digital signature
