On Tue, Oct 26, 2010 at 1:03 AM, Joe Eykholt <[email protected]> wrote: > > > On 10/25/10 6:26 AM, Hillf Danton wrote: >> On Mon, Oct 25, 2010 at 2:28 PM, Joe Eykholt <[email protected]> wrote: >>> >>> >>> On 10/24/10 11:17 PM, Joe Eykholt wrote: >>>> >>>> >>>> On 10/23/10 12:20 AM, Hillf Danton wrote: >>>>> >>>>> On Sat, Oct 23, 2010 at 2:53 AM, Joe Eykholt<[email protected]> wrote: >>>>>> >>>>>> On 10/22/10 8:04 AM, Hillf Danton wrote: >>>>>>> >>>>>>> There seems memset could be removed since the frame, returned by >>>>>>> fc_frame_alloc_fill with the filled bytes not zeroed, could reach its >>>>>>> receiver on the other end. >>>>>> >>>>>> That's why the memset is there, just to make sure the padding is zero. >>>>>> >>>>>> Otherwise, up to 3 bytes of uninitialized data could be sent. >>>>>> That seems like a very minor security problem, but still would be wrong >>>>>> to leak any uninitialized data, and it just seems nicer if the pad bytes >>>>>> are zero, even though they can be anything at all. >>>>>> >>>>>> What may be confusing to some readers is that we use skb_trim() to >>>>>> trim the length to not include the pad, but later in fc_exch_setup_hdr() >>>>>> we add the pad back (without zeroing it at that point). >>>>>> >>>>>> So, this patch should not be applied. >>>>>> >>>>> >>>>> The padded is not zeroed. >>>>> The zeroed lies in the range of payload whose len is payload_len. >>>> >>>> Not true. The padding starts at fc_hdr + payload len, and goes for fill >>>> bytes. >>> >>> I meant to say the *zeroing* starts ... >> >> Then why is skbuff trimmed to >> payload_len + sizeof(struct fc_frame_header) ? > > I explained this above at "What may be confusing ..." > Maybe it would be better to move the memset() > to fc_exch_setup_hdr(). It makes more sense there now.
Node and thanks Joe. //Hillf > >> >>> >>>>>>> >>>>>>> Signed-off-by: Hillf Danton<[email protected]> >>>>>>> --- >>>>>>> >>>>>>> --- a/drivers/scsi/libfc/fc_frame.c 2010-09-13 07:07:38.000000000 >>>>>>> +0800 >>>>>>> +++ b/drivers/scsi/libfc/fc_frame.c 2010-10-22 23:00:24.000000000 >>>>>>> +0800 >>>>>>> @@ -81,7 +81,6 @@ struct fc_frame *fc_frame_alloc_fill(str >>>>>>> fill = 4 - fill; >>>>>>> fp = _fc_frame_alloc(payload_len + fill); >>>>>>> if (fp) { >>>>>>> - memset((char *) fr_hdr(fp) + payload_len, 0, fill); >>>>>>> /* trim is OK, we just allocated it so there are no >>>>>>> fragments */ >>>>>>> skb_trim(fp_skb(fp), >>>>>>> payload_len + sizeof(struct fc_frame_header)); >>>>>>> _______________________________________________ >>>>>>> devel mailing list >>>>>>> [email protected] >>>>>>> http://www.open-fcoe.org/mailman/listinfo/devel >>>>>> >>>> _______________________________________________ >>>> devel mailing list >>>> [email protected] >>>> http://www.open-fcoe.org/mailman/listinfo/devel >>> > _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
